| Message ID | 20210320145706.12308-1-jlicht@fsfe.org |
|---|---|
| Headers | show |
| Series | node going forward | expand |
Hi,
> So, some people seem to be interested in this one; please review and test.
that would be me :)
The first patch currently does not apply cleanly to master, but it’s an easy
merge.
There is a stray “wip” in the commit message of the last patch.
I imported (using the “binary” importer from wip-node-14 and some private
fixes) and built more than 18000 NPM packages with this patchset and the
resulting RStudio is working fine, so this is a welcome improvement for
guix-science.
Cheers,
Lars
On Tue, 2021-03-23 at 10:05 +0100, Lars-Dominik Braun wrote: > I imported (using the “binary” importer from wip-node-14 and some > private > fixes) and built more than 18000 NPM packages with this patchset and > the > resulting RStudio is working fine, so this is a welcome improvement > for > guix-science. There's somewhere with 18000+ GNU Guix package definitions for NPM packages? Please share where. Thank you
Hi Léo, > There's somewhere with 18000+ GNU Guix package definitions for NPM > packages? Please share where. sorry, the number is completely wrong, no idea why. It’s “just” 473 packages, which is still an insane amount. They’re part of my efforts to build RStudio 1.4, see https://github.com/guix-science/guix-science/blob/rstudio-1.4/guix-science/packages/rstudio-node.scm Lars
Hi Jelle,
Jelle Licht <jlicht@fsfe.org> writes:
> So, some people seem to be interested in this one; please review and test.
Now that I’ve finally taken the time to dig into what you’ve done here –
I must say it’s very impressive!
I’ve taken the presumptuous step of re-rolling the series. The reason
is that all the “(delete 'build)” bits were bothering me. I decided to
have the build system check the “package.json” file for a build script
before trying to run it. Since that change required changing all the
other patches, I thought it would be easier to just post the updated
patches. Also, I’m hoping to spare you some trouble (since you’ve
already gone to a lot!).
Of course, this approach gave me free reign to pick nits. :) Below is
a list of bigger things that I changed, but I also adjusted some commit
messages, indentation, descriptions, and other minor things.
• Add the check for a “build” script as explained above, and adjust
the “npm-build-system” packages accordingly.
• Rename “libuv-node” to “libuv-for-node”, as this style is used for
similar packages. I also changed the name to just “libuv” and
marked it hidden.
• Change the “Fix incorrect import semantics” comments to “Fix
imports for esbuild”. To me, if TypeScript’s tsc likes the
imports, they are correct TypeScript (despite the esbuild bug
report).
• Set the llhttp version to 2.1.3, and add a patch to fix
CVE-2020-8287. The resulting C source files are identical to the
ones shipped with Node.js 14.16.0. This makes the tests a little
simpler, allowing the removal of the HTTP method superset change
and fixing the reading one byte failure.
• Fix the SIGXFSZ failure by fixing a “/bin/sh” in the test.
The final result is still a little messy, but I don’t think we should
hold this back any longer. It’s a significant step forward, and it puts
us in better shape to improve things incrementally.
WDYT? Let me know if I made anything worse! :) If the altered patches
look good to you, I suggest you go ahead and push them.
-- Tim
Timothy, Timothy Sample <samplet@ngyro.com> writes: > Hi Jelle, > > Jelle Licht <jlicht@fsfe.org> writes: > >> So, some people seem to be interested in this one; please review and test. > > Now that I’ve finally taken the time to dig into what you’ve done here – > I must say it’s very impressive! If you bang your head against a wall often enough, it will crack eventually. Head or wall, either way works in this metaphor ;-). > I’ve taken the presumptuous step of re-rolling the series. The reason > is that all the “(delete 'build)” bits were bothering me. I decided to > have the build system check the “package.json” file for a build script > before trying to run it. Since that change required changing all the > other patches, I thought it would be easier to just post the updated > patches. Also, I’m hoping to spare you some trouble (since you’ve > already gone to a lot!). Makes sense, thanks! Please be presumptuous as often as you'd like. > > • Change the “Fix incorrect import semantics” comments to “Fix > imports for esbuild”. To me, if TypeScript’s tsc likes the > imports, they are correct TypeScript (despite the esbuild bug > report). "Something a native speaker of English can make sense of" != "Proper English", and in that same vein I don't think a commmon mistake with workaround in place is not a mistake. I really don't care about what ends up in the codebase though, as long as it is clear why we do what we do, which works out just fine with your comment. > The final result is still a little messy, but I don’t think we should > hold this back any longer. It’s a significant step forward, and it puts > us in better shape to improve things incrementally. > > WDYT? Let me know if I made anything worse! :) If the altered patches > look good to you, I suggest you go ahead and push them. I still adressed some of Efraim's remarks, and pushed it to master just now. There are quite some ways to go from here: * Get the 'binary' importer upstreamable (I will continue with this) * Properly support cross-compilation of Node and Node-packages I had a super quick look at this, but it seems that in building node, you build intermediate tools that run on the host. Perhaps some our x-compilation gurus can weigh in. * Make a Rome-based build system, once Rome does more than linting, to help untangle the knot that is JavaScript-packaging But for today (and the upcoming release), modern Node on guix \o/ Thanks folks! - Jelle
Hi, Jelle Licht <jlicht@fsfe.org> writes: > Timothy Sample <samplet@ngyro.com> writes: > >> • Change the “Fix incorrect import semantics” comments to “Fix >> imports for esbuild”. To me, if TypeScript’s tsc likes the >> imports, they are correct TypeScript (despite the esbuild bug >> report). > > "Something a native speaker of English can make sense of" != "Proper > English", and in that same vein I don't think a commmon mistake with > workaround in place is not a mistake. > > I really don't care about what ends up in the codebase though, as long > as it is clear why we do what we do, which works out just fine with your > comment. Heh. You’re right: it’s not a big deal. Thanks for humouring me. :) >> The final result is still a little messy, but I don’t think we should >> hold this back any longer. It’s a significant step forward, and it puts >> us in better shape to improve things incrementally. >> >> WDYT? Let me know if I made anything worse! :) If the altered patches >> look good to you, I suggest you go ahead and push them. > > I still adressed some of Efraim's remarks, and pushed it to master just > now. Nice!! > There are quite some ways to go from here: > > * Get the 'binary' importer upstreamable (I will continue with this) > > * Properly support cross-compilation of Node and Node-packages > > I had a super quick look at this, but it seems that in building node, > you build intermediate tools that run on the host. Perhaps some our > x-compilation gurus can weigh in. > > * Make a Rome-based build system, once Rome does more than linting, to > help untangle the knot that is JavaScript-packaging Sounds pretty exciting! -- Tim