feat: rebuild on external extra dep change #49
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
warner
approved these changes
Jun 30, 2024
There was a problem hiding this comment.
huh, so this gives us the means to force a rebuild (assuming Makefile variables get overridden by environment variables, which I think is correct, for this syntax). But it doesn't embed a version in the binary. But, it's not like the stale -10 version that used to be there was doing us any good. So I think this is ok.
warner
approved these changes
Jun 30, 2024
There was a problem hiding this comment.
Oh, now I see the -v, good.
Could you add a line that concatenates a greppable string like "xsnap version" with the -D -supplied version, and store the result in an (unused) static string? That will let us extract the version with /bin/strings (without trying to execute xsnap). I think the syntax is like:
char *VERSION = "xsnap version: XSNAP_VERSION";but it might involve something like foo ## XSNAP_VERSION
Then run /usr/bin/strings on the compiled xsnap and grep for "xsnap version:" and we should see the version string.
Done |
warner
approved these changes
Jul 1, 2024
There was a problem hiding this comment.
looks good, minor nit: xsnap-version instead of xsnap_version, and a comment next to char *VERSION like "this static string lets "strings path/to/xsnap |grep xsnap-version" work, even without executing xsnap -v", because sooner or later somebody will notice that VERSION isn't used.
I will keep the
Aye! |
mhofman
force-pushed
the
mhofman/9614-build-env
branch
from
9b1a4a4
to
8f39ab0
Compare
mergify bot
added a commit
to Agoric/agoric-sdk
that referenced
this pull request
Jul 1, 2024
closes: #9614 closes: #7012 refs: agoric-labs/xsnap-pub#49 ## Description Wire a mechanism to force rebuilds of xsnap when some build environments change, or if the xsnap binary is found to be out of date through the embedded xsnap package version. Moves the `agd build` check to depend on a version check implemented by the xsnap package, which now only depends on the package version that is dynamically inserted instead of an explicit magic string that wasn't getting updated. #7012 (comment) expressed concerns with relying on the xsnap package version, however: - any changes to `xsnap-pub` or `moddable` submodules would result in a change to the checked in `build.env` file, which would be detected by `lerna` during the publish process. While a version bump would not apply for contributors of the sdk (aka not every xsnap change results in a version change), it will apply for anyone using published versions, even if the change is in submodules only. - By having the version check look up the expected version from the `package.json` file directly, we avoid having to modify both the `package.json` file and the check itself. Only the automatically generated single publish commit will trigger a forced rebuild, and satisfy the check on its own, - At the same time we remove the dependency of `agd build` onto the the internal structure of xsnap. ### Security Considerations Forces all validators to use the expected version of xsnap ### Scaling Considerations Does a few more checks when building xsnap, and causes full rebuilds in some cases where they might not be strictly necessary. ### Documentation Considerations Should all be transparent ### Testing Considerations Manually tested by touching the xsnap package version, or reverting just the release binary to a previous copy (or deleting altogether). If the binary is meddled with like this, `agd build` checks will fail, but a manual `yarn build` will fix. I thought this was better than transparently forcing a rebuild in that abnormal situation. ### Upgrade Considerations Smoother upgrades by validators
mhofman
pushed a commit
to Agoric/agoric-sdk
that referenced
this pull request
Jul 1, 2024
closes: #9614 closes: #7012 refs: agoric-labs/xsnap-pub#49 ## Description Wire a mechanism to force rebuilds of xsnap when some build environments change, or if the xsnap binary is found to be out of date through the embedded xsnap package version. Moves the `agd build` check to depend on a version check implemented by the xsnap package, which now only depends on the package version that is dynamically inserted instead of an explicit magic string that wasn't getting updated. #7012 (comment) expressed concerns with relying on the xsnap package version, however: - any changes to `xsnap-pub` or `moddable` submodules would result in a change to the checked in `build.env` file, which would be detected by `lerna` during the publish process. While a version bump would not apply for contributors of the sdk (aka not every xsnap change results in a version change), it will apply for anyone using published versions, even if the change is in submodules only. - By having the version check look up the expected version from the `package.json` file directly, we avoid having to modify both the `package.json` file and the check itself. Only the automatically generated single publish commit will trigger a forced rebuild, and satisfy the check on its own, - At the same time we remove the dependency of `agd build` onto the the internal structure of xsnap. ### Security Considerations Forces all validators to use the expected version of xsnap ### Scaling Considerations Does a few more checks when building xsnap, and causes full rebuilds in some cases where they might not be strictly necessary. ### Documentation Considerations Should all be transparent ### Testing Considerations Manually tested by touching the xsnap package version, or reverting just the release binary to a previous copy (or deleting altogether). If the binary is meddled with like this, `agd build` checks will fail, but a manual `yarn build` will fix. I thought this was better than transparently forcing a rebuild in that abnormal situation. ### Upgrade Considerations Smoother upgrades by validators
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refs: Agoric/agoric-sdk#9614
Add an
EXTRA_DEPSenvironment to the makefile to enable agoric-sdk to trigger a rebuild if some env options change likeXSNAP_VERSIONorCC.Drive-by deprecation of the
-ncommand since the agoric-sdk PR will get rid of it in favor of checking theXSNAP_VERSIONalready embedded in-vTested by building both manually invoking the makefile with and without the
EXTRA_DEPSset, and through the build script changed in agoric-sdk.