Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node: Add support for Git sources with lockfile v2 processing #351

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

refi64
Copy link
Collaborator

@refi64 refi64 commented Feb 24, 2023

Note that I left

# TODO Once lockfile v2 syntax support is complete, use _process_packages_v2 for
# both v2 and v3 lockfiles

because using v2 processing for v2 lockfiles breaks missing resolved fields.

(I'm not convinced we should rely solely on the git-to-tarball conversions npm does: losing support for custom Git services isn't great IMO.)

@gasinvein
Copy link
Member

gasinvein commented Feb 25, 2023

I'm not convinced we should rely solely on the git-to-tarball conversions npm does: losing support for custom Git services isn't great IMO

I believe it would be a fair tradeoff. Our git handling is extremely messy and includes terrible hacks, i.e. dynamic package.json patching at build time.
Custom git hosts are so rare (I've never seen a single one in the wild) that dropping support for them, IMO, totally worth the code simplicity and reliability gain.

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>
@refi64 refi64 force-pushed the wip/refi64/lockfile-v2-git branch from c3a96d3 to 21bb9e9 Compare March 11, 2023 01:04
@refi64
Copy link
Collaborator Author

refi64 commented Mar 11, 2023

Our git handling is extremely messy and includes terrible hacks, i.e. dynamic package.json patching at build time.

I mean, we're also building the entire internal cache representation from scratch, I'm not sure if the git handling really stands out here...

Custom git hosts are so rare (I've never seen a single one in the wild)

Probably worth noting that this includes self-hosted GitHub Enterprise and GitLab, not just fully alternative git hosts. There are also a handful of Electron apps on smaller, currently unsupported instances like Codeberg, and we already have some apps that pull in their own internal deps over git, so I don't think it's unreasonable to believe that someone would eventually combine those two together.

@gasinvein
Copy link
Member

gasinvein commented Mar 22, 2023

I'm not sure if the git handling really stands out here

IMO it does. Massive jq-driven build-time patches are significantly more complex (and probably prone to break) than plain and simple flatpak-builder sources that don't need any post-processing.

this includes self-hosted GitHub Enterprise and GitLab

Hmm, does npm detect custom GitLab hosts? If it does, can we do the same?

@refi64
Copy link
Collaborator Author

refi64 commented Mar 24, 2023

IMO it does. Massive jq-driven build-time patches are significantly more complex (and probably prone to break) than plain and simple flatpak-builder sources that don't need any post-processing.

Fair, tbh if part of the issue is it being in a somewhat strange syntax, it could really just as easily be a Python script. (To be entirely frank, I'm not sure why I went jq here in the first place...)

Hmm, does npm detect custom GitLab hosts? If it does, can we do the same?

From what I can tell, this is explicitly not supported in hosted-git-info, and I can't find anything inside npm that would imply it does anything extra here.

@gasinvein
Copy link
Member

IMO it does. Massive jq-driven build-time patches are significantly more complex (and probably prone to break) than plain and simple flatpak-builder sources that don't need any post-processing.

Fair, tbh if part of the issue is it being in a somewhat strange syntax, it could really just as easily be a Python script. (To be entirely frank, I'm not sure why I went jq here in the first place...)

The jq syntaix is fine, I actually love jq. What IMO is not fine is the sheer size of the generated scripts to run at build time. If we can potentially avoid them altogether in 99% case, it seems like something worth a try?

Hmm, does npm detect custom GitLab hosts? If it does, can we do the same?

From what I can tell, this is explicitly not supported in hosted-git-info, and I can't find anything inside npm that would imply it does anything extra here.

Ideally, I believe we should use generated tarballs where NPM uses them, a fall back to git where it does.

@jwillikers
Copy link

I need support for this for the latest release of Stretchly. Attempting to use this results in the following error when building the Flatpak:

Downloading sources
Fetching git repo git+ssh://git@github.com/Jelmerro/dbus-final.git, ref refs/heads/master
remote: Total 0 (delta 0), reused 0 (delta 0), pack-reused 0
Fetching git repo https://github.com/hovancik/stretchly.git, ref refs/tags/v1.15.0
remote: Total 0 (delta 0), reused 0 (delta 0), pack-reused 0
Starting build of net.hovancik.Stretchly
Cache hit for dbus-glib, skipping build
Cache miss, checking out last cache hit
========================================================================
Building module stretchly in /home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-12
========================================================================
Note: switching to '3e43f60d80bbcdf0dfa0b59b838097d6af4d17ba'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 3e43f60 update deps
jq: error: Could not open file /var/home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-12/package.json: No such file or directory
jq: error: Could not open file /var/home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-12/package-lock.json: No such file or directory
/var/home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-12/flatpak-node/patch/app.sh: line 2: /var/home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-12/app/package.json.new: No such file or directory
mv: cannot stat '/var/home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-12/app/package.json.new': No such file or directory
/var/home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-12/flatpak-node/patch/app.sh: line 4: /var/home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-12/app/package-lock.json.new: No such file or directory
mv: cannot stat '/var/home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-12/app/package-lock.json.new': No such file or directory
Error: module stretchly: Child process exited with code 1

@hfiguiere hfiguiere added the node label Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants