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] support git sources in lockfile v2 format #382

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Ian2020
Copy link

@Ian2020 Ian2020 commented Nov 3, 2023

Fixes #381 and I hope will help #237 and #182.

I've added a new set of tests to cover more types of git references: https and ssh both with a commit hash and without. The tests cover all versions of the lockfile format v1,2,3.

In my testing I found that it doesn't appear to be necessary to patch package-lock.json (npm.py:444), so that has been removed. If there's another reason that is needed let me know but the tests all pass without it.

@refi64
Copy link
Collaborator

refi64 commented Nov 3, 2023

I think this has some overlap with #351 😅 see the discussion there.

@jwillikers
Copy link

Attempting to use this for the Stretchly flatpak, and I get the following error when building:

Downloading sources
Fetching git repo https://[email protected]/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-11
========================================================================
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-11/package.json: No such file or directory
/var/home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-11/flatpak-node/patch/app.sh: line 2: /var/home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-11/app/package.json.new: No such file or directory
mv: cannot stat '/var/home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-11/app/package.json.new': No such file or directory
Error: module stretchly: Child process exited with code 1

@Ian2020
Copy link
Author

Ian2020 commented Nov 12, 2023

There's something up with the paths here:

Building module stretchly in /home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-11

...but later the jq is looking in /var/home/... rather than /home/...

jq: error: Could not open file /var/home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-11/package.json: No such file or directory

Not sure where that /var prefix has come from.

@jwillikers
Copy link

jwillikers commented Nov 12, 2023

There's something up with the paths here:

Building module stretchly in /home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-11

...but later the jq is looking in /var/home/... rather than /home/...

jq: error: Could not open file /var/home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-11/package.json: No such file or directory

Not sure where that /var prefix has come from.

That's a thing on Fedora rpm-ostree systems. /home is a symlink to /var/home. This is documented here.

@jwillikers
Copy link

The manifest and everything can be found in this MR.

@Ian2020
Copy link
Author

Ian2020 commented Nov 12, 2023

Thanks, I'll take a look.

@Ian2020
Copy link
Author

Ian2020 commented Nov 13, 2023

Ok a few things going on here:

  • The inclusion of generated-sources.json in net.hovancik.Stretchly.yaml needs to come AFTER the stretchly source code is checked out, that's why package.json can't be found to be patched by generated-sources. This should fix the error you've reported but you'll next hit a ENOTCACHED error because...
  • We don't patch refs that looks like "github:user/project" and in Stretchly you have exactly this: "github:Jelmerro/dbus-final". I can't find in the package.json spec where this is allowed but it seems to be common searching across Github. So I think I have to add support for that to this PR.
  • The Stretchly project has two package.json files: one in / one in /app. Both would have to be patched for its flatpak build to succeed with git refs. The electron-builder cmd in the postinstall script seems to use the /app one. @jwillikers do you know why there are two package.json files?

@jwillikers
Copy link

Ok a few things going on here:

* The inclusion of `generated-sources.json` in [net.hovancik.Stretchly.yaml](https://github.com/flathub/net.hovancik.Stretchly/blob/master/net.hovancik.Stretchly.yaml) needs to come AFTER the stretchly source code is checked out, that's why package.json can't be found to be patched by generated-sources. This should fix the error you've reported but you'll next hit a ENOTCACHED error because...

* We don't patch refs that looks like "github:user/project" and in Stretchly you have exactly this: "github:Jelmerro/dbus-final". I can't find in the [package.json spec](https://docs.npmjs.com/cli/v9/configuring-npm/package-json) where this is allowed but it seems to be common searching across Github. So I think I have to add support for that to this PR.

* The Stretchly project has two `package.json` files: one in `/` one in `/app`. Both would have to be patched for its flatpak build to succeed with git refs. The `electron-builder` cmd in the postinstall script seems to use the `/app` one. @jwillikers do you know why there are two `package.json` files?

I've changed that order.
As for why there are two package.json files, I'm not sure.
@hovancik Could you shed some light on why there is app/package.json in addition to package.json?

@hovancik
Copy link

@jwillikers that's the structure from electron-builder: https://www.electron.build/tutorials/two-package-structure.html

I've tried in the past to remove 2 package structure, as they say it's not needed anymore but it did not work for me.

@Ian2020
Copy link
Author

Ian2020 commented Nov 16, 2023

  • We don't patch refs that looks like "github:user/project" and in Stretchly you have exactly this: "github:Jelmerro/dbus-final". I can't find in the package.json spec where this is allowed but it seems to be common searching across Github. So I think I have to add support for that to this PR.

Ok I've added support for these 'shortcut' git urls to the PR. But we still need to overcome the 2 package structure in stretchly somehow.

@hovancik
Copy link

@Ian2020 should I try to remove 2package structure? It's been some time since last time I tried so I can try again if that helps

@Ian2020
Copy link
Author

Ian2020 commented Nov 16, 2023

@hovancik yes I think that would be simplest if you can, thanks.

@hovancik
Copy link

@Ian2020 done, new version is out.

@hovancik
Copy link

@Ian2020 actually scratch that, it does not work, will need to look into this more.

@Ian2020
Copy link
Author

Ian2020 commented Nov 20, 2023

Ok, despite that I can now successfully build Stretchly v1.15.1 as a flatpak. I created generated-sources.yaml using this PR against v1.15.1 of stretchly. Then copied that file into branch update-71b455c of https://github.com/flathub/net.hovancik.Stretchly and updated the source tag in the yaml to v1.15.1 and it builds a flatpak that I could run @hovancik @jwillikers

So I think this PR is doing what it should do for Stretchly to build as a flatpak, I hope you can sort the other issues in Stretchly. Let me know if you think there's anything more needed here.

@jwillikers
Copy link

jwillikers commented Nov 20, 2023

@Ian2020 One thing, I noticed that running flatpak-node-generator from this PR doesn't work when attempting to run it from a relative directory. I usually run flatpak-node-generator npm stretchly/package-lock.json --electron-node-headers with Stretchly checked out in the stretchly subdirectory in the Flatpak repository. I had to change into the stretchly subdirectory, generate the sources, and then move the generated file up one directory to make things work.

@hovancik
Copy link

@Ian2020 can you possibly try with this PR? hovancik/stretchly#1401

Seems like it was local issue only but would be nice to confirm build is ok after this change

@jwillikers
Copy link

@Ian2020 can you possibly try with this PR? hovancik/stretchly#1401

Seems like it was local issue only but would be nice to confirm build is ok after this change

@hovancik I just tested it and that commit builds and runs fine.

@hovancik
Copy link

Thanks, we are all good then, I guess.

p2004a added a commit to p2004a/info.beyondallreason.bar that referenced this pull request Mar 3, 2024
As part of the update, I had to refactor quite a bit of how the build
process works.

flatpak-node-generator is quite bugged:
 - Output `generated-source.json` expects the package to be in the main
   directory, so I've split the build into modules but still had to add
   `flatpak-node` exclusion to build
 - I had to patch locally flatpak/flatpak-builder-tools#382
   and use that because flatpak/flatpak-builder-tools#381
 - I've also run into flatpak/flatpak-builder-tools#377

I've also:
 - Made copying of addr2line dependencies more reliable
 - Merged the startup scripts into one
 - Added permission and setup for discord so that rich presence works
p2004a added a commit to p2004a/info.beyondallreason.bar that referenced this pull request Mar 3, 2024
As part of the update, I had to refactor quite a bit of how the build
process works.

flatpak-node-generator is quite bugged:
 - Output `generated-source.json` expects the package to be in the main
   directory, so I've split the build into modules but still had to add
   `flatpak-node` exclusion to build
 - I had to patch locally flatpak/flatpak-builder-tools#382
   and use that because flatpak/flatpak-builder-tools#381
 - I've also run into flatpak/flatpak-builder-tools#377

I've also:
 - Made copying of addr2line dependencies more reliable
 - Merged the startup scripts into one
 - Added permission and setup for discord so that rich presence works
p2004a added a commit to flathub/info.beyondallreason.bar that referenced this pull request Mar 3, 2024
As part of the update, I had to refactor quite a bit of how the build
process works.

flatpak-node-generator is quite bugged:
 - Output `generated-source.json` expects the package to be in the main
   directory, so I've split the build into modules but still had to add
   `flatpak-node` exclusion to build
 - I had to patch locally flatpak/flatpak-builder-tools#382
   and use that because flatpak/flatpak-builder-tools#381
 - I've also run into flatpak/flatpak-builder-tools#377

I've also:
 - Made copying of addr2line dependencies more reliable
 - Merged the startup scripts into one
 - Added permission and setup for discord so that rich presence works
@proletarius101
Copy link
Contributor

Hi @hfiguiere. Since you mentioned that you prefer to build from source, could this PR be merged?

@hfiguiere
Copy link
Collaborator

I don't maintain that module and I don't have the knowledge space.

Co-authored-by: Danilo Bargen <[email protected]>
then
to_entries | map(
if (.value | type == "string") and $data[.value]
then .value = "git+file:\($buildroot)/\($data[.value])"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this fork to update the dependencies for the Stretchly Flatpak resulted in the following error:

npm error code 128
npm error An unknown git error occurred
npm error command git --no-replace-objects ls-remote file:/var/home/jordan/Projects/net.hovancik.Stretchly/.flatpak-builder/build/stretchly-14/flatpak-node/git-packages/dbus-final-3e43f60d80bbcdf0dfa0b59b838097d6af4d17ba
npm error ssh: Could not resolve hostname file: Temporary failure in name resolution
npm error fatal: Could not read from remote repository.
npm error
npm error Please make sure you have the correct access rights
npm error and the repository exists.

I was able to fix this by using the syntax file:// instead of just file:.
This matches the documented syntax for Git URL's.

Suggested change
then .value = "git+file:\($buildroot)/\($data[.value])"
then .value = "git+file://\($buildroot)/\($data[.value])"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[node] NotImplementedError: Git sources in lockfile v2 format are not supported yet
7 participants