[node] support git sources #382
Conversation
|
I think this has some overlap with #351 😅 see the discussion there. |
|
Attempting to use this for the Stretchly flatpak, and I get the following error when building: |
|
There's something up with the paths here: ...but later the jq is looking in Not sure where that |
That's a thing on Fedora rpm-ostree systems. |
|
The manifest and everything can be found in this MR. |
|
Thanks, I'll take a look. |
|
Ok a few things going on here:
|
I've changed that order. |
|
@jwillikers that's the structure from 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
force-pushed
the
gitrefs
branch
2 times, most recently
from
3cd5b3e to
f28defd
Compare
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. |
|
@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 |
|
@hovancik yes I think that would be simplest if you can, thanks. |
|
@Ian2020 done, new version is out. |
|
@Ian2020 actually scratch that, it does not work, will need to look into this more. |
|
Ok, despite that I can now successfully build Stretchly v1.15.1 as a flatpak. I created 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. |
|
@Ian2020 One thing, I noticed that running |
|
@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. |
|
Thanks, we are all good then, I guess. |
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
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
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
|
Hi @hfiguiere. Since you mentioned that you prefer to build from source, could this PR be merged? |
|
I don't maintain that module and I don't have the knowledge space. |
|
Today's commit addresses all issues raised so far. I can build both Stretchly and Pocket-Casts flatpaks no problem. Please let me know of any new problems. About these latest changes:
|
Ian2020
changed the title
This is due to flatpak/flatpak-builder-tools#381 Their PR flatpak/flatpak-builder-tools#382 implements the required functionality.
This is due to flatpak/flatpak-builder-tools#381 Their PR flatpak/flatpak-builder-tools#382 implements the required functionality.
This is due to flatpak/flatpak-builder-tools#381 Their PR flatpak/flatpak-builder-tools#382 implements the required functionality.
|
This is blocking updating pocket-casts: And it fixes issues that otherwise cause dependency generation not to work. Using Ian2020's repo didn't cause problems 2 weeks ago (flathub/tech.feliciano.pocket-casts#8 (comment)), but it now does. Let me know if I should file an issue about this (which I definitely will if this cannot not be fixed quickly). (Note to self, the repo to file it against is https://github.com/flathub-infra/vorarbeiter/) |
|
@hadess Instead of using Ian2020's fork, you could just reference this PR. To e.g. install it via pipx: pipx install --force git+https://github.com/flatpak/flatpak-builder-tools.git@refs/pull/382/head#subdirectory=node |
|
External gitmodule check was added to linter several months ago and it doesn't have anything to do with vorarbeiter. |
Submodules outside the sanctioned organisations are not allowed, so install the fixed flatpak-node-generator through its fork directly. See flatpak/flatpak-builder-tools#382 (comment)
Submodules outside the sanctioned organisations are not allowed, so install the fixed flatpak-node-generator through its fork directly. See flatpak/flatpak-builder-tools#382 (comment)
This worked (and I learnt something), thanks! |
Submodules outside the sanctioned organisations are not allowed, so install the fixed flatpak-node-generator through its fork directly. See flatpak/flatpak-builder-tools#382 (comment)
@bbhtt can do, probably be next week now |
|
will take a look again sometime this week |
Done. |
| self.gen.add_git_source(source.url, source.commit, path) | ||
|
|
||
| url = urllib.parse.urlparse(source.url) |
There was a problem hiding this comment.
is the git source here always going to github.com and gitlab.com? because only those two seems to be handled.
There was a problem hiding this comment.
When I was working on this github and gitlab were the only two forges on which I could easily find packages to test, however npm seems to also support bitbucket and gists from their docs: https://docs.npmjs.com/cli/v11/commands/npm-install.
I agree the code should be fixed though as any other url will fail. Is it ok to throw an exception if it's not a url we can handle? My suspicion is that gitlab/github covers 80%+ of use cases. If anyone can provide a failing url it could be addressed in future as we need to determine where to get the tarball from and how npm wants it indexed.
There was a problem hiding this comment.
s it ok to throw an exception if it's not a url we can handle?
Ok let's do that for now.
There was a problem hiding this comment.
I believe the full list of stuff to handle would be in: https://github.com/npm/hosted-git-info
Co-authored-by: Danilo Bargen <[email protected]> Co-authored-by: Jordan Williams <[email protected]>
| # with https://[email protected]/ianstormtaylor/to-camel-case.git | ||
| # for git+ssh URLs | ||
| if ':' in new_url.netloc: | ||
| netloc_split = new_url.netloc.split(':') |
There was a problem hiding this comment.
this will break if there are extra colons (I mean, there really shouldn't be, but alas)
| netloc_split = new_url.netloc.split(':') | |
| netloc_split = new_url.netloc.split(':', 1) |
(note that the branch immediately above also handles this, but...it does so kinda poorly, by comparison)
| source = ResolvedSource( | ||
| resolved=info['resolved'], integrity=integrity | ||
| ) | ||
| if info['resolved'].startswith('git+'): |
There was a problem hiding this comment.
info['resolved'] should probably be in an intermediate variable so the same string constant isn't copy-pasted in 3 places. (this is also something the rest of the code admittedly does poorly.)
| self.gen.add_git_source(source.url, source.commit, path) | ||
|
|
||
| url = urllib.parse.urlparse(source.url) |
There was a problem hiding this comment.
I believe the full list of stuff to handle would be in: https://github.com/npm/hosted-git-info
| @@ -0,0 +1,94 @@ | |||
| { | |||
There was a problem hiding this comment.
very nice to see the tests for this!
| ), | ||
| commands=[ | ||
| provider_factory_spec.install_command, | ||
| """node -e 'require("array-range");require("is-empty-object");require("is-number");require("person-lib");require("to-camel-case");require("to-capital-case");require("to-no-case");require("to-space-case");require("@flatpak-node-generator-tests/subdir").sayHello()'""", |
There was a problem hiding this comment.
this is...a bit horrifying, you could pull out the JS into a top-level multiline string constant, or just stick it in the shared datadir with the package.json (we don't really do that anywhere else, but the other lines are also much shorter!).
| url = urllib.parse.urlparse(source.url) | ||
| if url.netloc == '[email protected]' or url.netloc == 'github.com': | ||
| url = url._replace( | ||
| netloc='codeload.github.com', path=re.sub('.git$', '', url.path) |
There was a problem hiding this comment.
should the . in .git here and below be escaped? might be useful to extract this into a separate git_suffix = re.compile(r'\.git$') and then use that for the substitutions below.
| netloc='codeload.github.com', path=re.sub('.git$', '', url.path) | ||
| ) | ||
| tarball_url = url._replace( | ||
| path=re.sub('$', f'/tar.gz/{source.commit}', url.path) |
There was a problem hiding this comment.
is this just...replacing the empty string? couldn't it just be a string concat then? ditto for gitlab.com below.
| self.gen.add_git_source(source.url, source.commit, path) | ||
|
|
||
| url = urllib.parse.urlparse(source.url) | ||
| if url.netloc == '[email protected]' or url.netloc == 'github.com': |
There was a problem hiding this comment.
could be url.hostname == 'github.com' (and maybe also check url.username in ('git', '') or similar? do you know if that's semantically important in any way? if not, then just checking the hostname is a bit cleaner.) ditto for the gitlab.com check below.
| @@ -208,7 +206,7 @@ def __init__( | |||
| self.all_lockfiles: Set[Path] = set() | |||
| # Mapping of lockfiles to a dict of the Git source target paths and GitSource objects. | |||
| self.git_sources: DefaultDict[ | |||
| Path, Dict[Path, GitSource] | |||
| Path, Dict[Path, Tuple[str, GitSource]] | |||
There was a problem hiding this comment.
should update the comment above to indicate what this is, since it's no longer just a GitSource. or maybe the value a dataclass like NamedGitSource or something instead, to make it clearer what the str is (and then you can just use attributes to get the innards, instead of having to do unpacking assignments)
| # Don't use with_extension to avoid problems if the package has a . in its name. | ||
| patch_dest = patch_dest.with_name(patch_dest.name + '.sh') | ||
| with open(lockfile) as fp: | ||
| lockfile_v1 = json.load(fp)['lockfileVersion'] == 1 |
There was a problem hiding this comment.
this could probably be saved inside self.git_sources' values, right? and then no need to re-open the lockfile here.
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.