fix(common): add proper configure output for hextobin #12440
Conversation
This change fixes a dependency problem when building Core. Previously this failed on a clean source tree because hextobin was missing dependencies but didn't run its `configure` action if the top-level `node_modules` folder did already exist.
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
|
Changes in this pull request will be available for download in Keyman version 18.0.115-alpha |
| @@ -11,7 +11,7 @@ THIS_SCRIPT="$(readlink -f "${BASH_SOURCE[0]}")" | |||
|
|
|||
| builder_describe "Build hextobin" clean configure build | |||
| builder_describe_outputs \ | |||
| configure /node_modules \ | |||
| configure /common/tools/hextobin/node_modules/commander \ | |||
There was a problem hiding this comment.
This is possibly fragile with hoisting of commander to top-level that could happen if we update commander versions. It could mean that it never seems configured.
Also not sure how this improves things, because:
Previously this failed on a clean source tree because hextobin was missing dependencies but didn't run its configure action if the top-level node_modules folder did already exist.
The /node_modules folder would not exist on a clean source tree; it should only exist if npm ci or npm i has already been run, at which point common/tools/hextobin/node_modules/... would also have been populated.
There was a problem hiding this comment.
I should say "half-clean" then. 😄 I often run a git clean on the subdirectory I'm working on in order to save some build time.
Yes, I realize that this is possibly fragile if we move files around and we can adjust then. Just checking for the existence of /node_modules is too broad IMO.
There was a problem hiding this comment.
Yes, I realize that this is possibly fragile if we move files around and we can adjust then.
The problem is that node_modules files can move around without us being very aware of it happening, because the choice of location is quite dependent on npm and whether or not we've hoisted packages (and if packages are shared across projects). It is very likely we'd simply miss the change and then we'd end up having the configure step running on every build until we notice the build slowdown.
Just checking for the existence of
/node_modulesis too broad IMO.
I guess so -- but it's what we are doing for all the other node/web projects, so this is a broader issue across many projects. We could consider having a separate '.configured' flag file created on each project as a more robust alternative that doesn't depend on vagaries of npm (and then we need to touch that file in build.sh of course):
| configure /common/tools/hextobin/node_modules/commander \ | |
| configure /common/tools/hextobin/.configured \ |
</bikeshed>
There was a problem hiding this comment.
Good discussion! See #12481 for a solution that picks up your idea.
This improves #12440. The previous change was a bit fragile if we updated commander versions. This change now implements a better solution that doesn't rely on dependencies and their locations for detecting if we need to run the `configure` action for hextobin.
This change fixes a dependency problem when building Core. Previously this failed on a clean source tree because hextobin was missing dependencies but didn't run its
configureaction if the top-levelnode_modulesfolder did already exist.@keymanapp-test-bot skip