-
Notifications
You must be signed in to change notification settings - Fork 524
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
Allow offline run of build:node script #2633
Conversation
Note: I'm not that versed in invocation of |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2633 +/- ##
==========================================
- Coverage 85.54% 84.97% -0.58%
==========================================
Files 76 81 +5
Lines 6858 7290 +432
==========================================
+ Hits 5867 6195 +328
- Misses 991 1095 +104 ☔ View full report in Codecov by Sentry. |
@KhafraDev since I think the use of npx was introduced in #2342. This seems like a reasonable tweak to handle the use case outlined by @khardix. |
It was added by @targos in 946ea78 The description may apply here (especially w/ nodejs/citgm#982)?:
|
Hm, no idea what AIX is, but your comment tells me this was a deliberate change and not easily reverted. Since I can patch this downstream without too much of an effort, I'm withdrawing this PR. Thanks for the explanation! |
@khardix AIX is one of the IBM platforms we help maintain in the upstream. I don't quite understand why one invocation of npx would work but not the other one. I believe that esbuild more recently started supporting AIX. I think do think its worth figuring out. Can you re-open the issue until we do that. We probably want to understand what the issue was on AIX anyway and if its no longer an issue, one less patch upstream is better. @abmusse can you confirm what I said about esbuild being recently update to support AIX? And possibly take a look to help us understand why one invocation works while the other does not. It might be an issue on AIX we at least want to be aware of. |
It's not that one invocation works while the other does not. It's that the |
It is, we test the node bundle in the fetch suite. |
Maybe it wasn't back in 2022 |
It used to be the case that |
ditto with Richard's comment above #2633 (comment) Version 0.19.10 added pre-complied binaries for AIX. I tested installing esbuild on an AIX machine and that worked. Comprehensive testing was not done though. |
From my perspective if we can somehow test that CITGM will run, then that would be best and would confirm that this change would be ok. |
This relates to...
#2342
Rationale
During packaging for a Linux distribution, the build systems are usually/sometimes disconnected from the internet,
and can not use anything not present in source tarball or installed on the system.
To facilitate NodeJS packages, in Fedora we cache the
node_modules
directory when creating a package update, and then re-use this cached directory for all subsequent builds. That means that we need all the build-time dependencies installed after invokingnpm install --include=dev
. This is currently not true for thebuild:node
script, which pullsesbuild@0.19.4
at the point of invocation. If invoked in offline mode (npm --offline run build:node
), this fails.Changes
devDependencies
, so that it is installed onnpm install --include=dev
.build:node
script.Status