Minor build improvements #614
Conversation
package.json
Outdated
| "prebuild": "run-s build.js && node ./script/prebuild.js", | ||
| "prebuild": "node ./script/prebuild.js", | ||
| "build.native": "node-gyp configure --release && node-gyp build --release", | ||
| "build.native.debug": "cross-env CMAKE_BUILD_TYPE=Debug node-gyp configure --debug && cross-env CMAKE_BUILD_TYPE=Debug node-gyp build --debug", | ||
| "build": "run-s build.js build.native", | ||
| "build.debug": "run-s build.js build.native.debug", |
There was a problem hiding this comment.
Why skipping the Js build before when it is needed?
There was a problem hiding this comment.
The preinstall hook is run before the install hook. That said, the scripts benefit very little from being written in TypeScript. I'd be in favor of converting them to plain JavaScript, or committing the compiled js to the repo so consumers can build the package from source without typescript installed.
script/build.ts
Outdated
| process.env.ZMQ_VERSION || "20de92ac0a2b2b9a1869782a429df68f93c3625e" | ||
| const src_url = `https://github.com/zeromq/libzmq/archive/${zmq_rev}.tar.gz` | ||
| process.env.ZMQ_VERSION || "4.3.5" | ||
|
|
||
| // if it looks like a dot-separated version number, prepend with "v" to match repo tagging convention | ||
| const gitref = zmq_rev.match(/^\d+\./) ? `v${zmq_rev}` : zmq_rev | ||
| const src_url = `https://github.com/zeromq/libzmq/archive/${gitref}.tar.gz` |
There was a problem hiding this comment.
I use an exact commit hash instead of tags because upstream is slow in tagging their releases.
There was a problem hiding this comment.
That makes a ton of sense and I love having the ability to have a hash. The version number is much more friendly. These changes still allow for a hash. I'll add a comment.
There was a problem hiding this comment.
Please change the title of the pull request to follow the Code of Conduct. We want to have a friendly environment here in Zeromq
https://zeromq.org/community/code-of-conduct/
rotu
force-pushed
the
literary-squid
branch
2 times, most recently
from
cc513dd
to
0c31bba
Compare
1. Don't skip tsc on install script. Previously, if `script/build.js` exists, we would skip the C++ build. 2. Fix issue with github tags if ZMQ version provided as semver. (github seems to normalize tarballed directory name by stripping leading `v` from tag) 3. Build scripts before node-gyp. Build library wrappers after.
I'm sorry if my words or tone seemed like trolling or like a personal attack! I don't at all mean to antagonize you or anyone else who pours selfless hours into open source software. I intended it to be levity over how frustrating it is to get a stable build process and an allusion to the frustrated confusion in recent issues. Thank you for the tone reminder. I'll try to be more constructive with my wording in the future! |
| // Revision to use. Can be a version number like "4.3.5", a branch name, or a commit hash. | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing, @typescript-eslint/strict-boolean-expressions | ||
| const zmq_rev = process.env.ZMQ_VERSION || "4.3.5" | ||
|
|
||
| // if it looks like a dot-separated version number, prepend with "v" to match repo tagging convention | ||
| const gitref = zmq_rev.match(/^\d+\./) ? `v${zmq_rev}` : zmq_rev | ||
| const src_url = `https://github.com/zeromq/libzmq/archive/${gitref}.tar.gz` |
There was a problem hiding this comment.
Given the conflicts and recent changes, this can be a separate pull request, but it needs some testing to verify it works
tscon install script. Previously, ifscript/build.jsexists, we would skip the C++ build.vfrom tag)Removed from this PR:
Remove questionable.npmrcSwitch to standardnode-gyp-build. Modified version tends to hang on failure instead of crash.