Skip to content
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

Don't run postinstall script separately from yarn install #629

Merged
merged 1 commit into from
Jun 9, 2023

Conversation

michalinacienciala
Copy link
Contributor

Background:
Previously we've been running postinstall script in a separate step from the yarn install command. We've been doing that as a workaround for a problem we had with the postinstall script in the @threshold-network/solidity-contracts package (the script in that package often randomly failed). Running yarn upgrade with --ignore-scripts flag prevented the execution of postinstall script in the main project and its depandencies. And running yarn run postinstall after did execute the postinstall script in the main project.

The change:
Recently we have refactored the @threshold-network/solidity-contracts project and got rid of the problematic script. We can go back to executing yarn install without the --ignore-scripts flag.

Ref:
threshold-network/solidity-contracts#142
threshold-network/solidity-contracts#143
threshold-network/token-dashboard#531

Background:
Previously we've been running postinstall script in a separate step from the
`yarn install` command. We've been doing that as a workaround for a problem we
had with the postinstall script in the `@threshold-network/solidity-contracts`
package (the script in that package often randomly failed). Running `yarn
upgrade` with `--ignore-scripts` flag prevented the execution of postinstall
script in the main project and its depandencies. And running `yarn run
postinstall` after did execute the postinstall script in the main project.

The change:
Recently we have refactored the `@threshold-network/solidity-contracts` project
and got rid of the problematic script. We can go back to executing `yarn
install` without the `--ignore-scripts` flag.
@dimpar dimpar merged commit 66a04c6 into main Jun 9, 2023
@dimpar dimpar deleted the no-longer-ignore-scripts branch June 9, 2023 09:09
@dimpar
Copy link
Contributor

dimpar commented Jun 9, 2023

Actually, now I have second thoughts about removing it. It was not running prepare-dependency.sh script and the reason why we have here yarn run postinstall might be a different one. If so, then we should not touch anything here. @lukasz-zimnoch can you please confirm if we can remove the --ignore-scripts flag here? Also, we have the same flag on line 34 @michalinacienciala

@lukasz-zimnoch
Copy link
Member

lukasz-zimnoch commented Jun 9, 2023

Actually, now I have second thoughts about removing it. It was not running prepare-dependency.sh script and the reason why we have here yarn run postinstall might be a different one. If so, then we should not touch anything here. @lukasz-zimnoch can you please confirm if we can remove the --ignore-scripts flag here? Also, we have the same flag on line 34 @michalinacienciala

The postinstall must be executed as tbtc-v2-monitoring depends on tbtc-v2 which in turn depends on bcoin that has a problematic bcrypto module that contains native code and must be rebuilt after yarn install. This has nothing to do with @threshold-network/solidity-contracts. I recommend reverting this PR as it breaks the Docker build.

@michalinacienciala
Copy link
Contributor Author

The postinstall must be executed as tbtc-v2-monitoring depends on tbtc-v2 which in turn depends on bcoin that has a problematic bcrypto module that contains native code and must be rebuilt after yarn install. This has nothing to do with @threshold-network/solidity-contracts. I recommend reverting this PR as it breaks the Docker build.

Well, I don't fully agree :) You're right that we need to run postinstall in order to execute npm rebuild bcrypto. But by default, postinstall scripts are executed during yarn-install (--frozen-lockfile). It's only because of the --ignore-scripts flag that they are not being executed and we need to run a separate yarn run postinstall command.
And the reason we were running the --ignore-scripts was due to the misbehaving prepare-dependency.sh postinstall script of threshold-network/solidity-contracts which is an indirect dependency of the monitoring package (see 5fbdc20).
Running yarn install --frozen-lockfile --ignore-scripts and then yarn run postinstall makes sure only the postinstall scripts in the monitoring project will be executed, no postinstall scripts in subdependencies will be run.

The reason I wanted to go back to executing all postinstall scripts, even in the subdependencies is because we got rid of the misbehaving prepare-dependencies.sh script in threshold-network/solidity-contracts and I was thinking we no longer need to worry about that. However, I did not think about two matters:

  1. I didn't update the yarn.lock and because we use --frozen-lockfile, still the old packages were used (including the threshold-network/solidity-contracts package that still contains the faulty script).
  2. Only the new development-tagged threshold-network/solidity-contracts package was updated to not contain the faulty script. The latest mainnet and goerli packages still have the script (we didn't publish a new packages there). And in monitoring dependencies we are using those packages. So we still could hit the issue with failing installation if we'd remove the --ignore-scripts flag.

So generally, I agree that my change was premature and should be reverted - I just wanted to clarify for future us what exactly was the reasoning for running the install with the --ignore-scripts flag.

michalinacienciala added a commit that referenced this pull request Jun 20, 2023
)

This reverts commit 2c7c63e.

See the following comment for a full description
#629 (comment)
@lukasz-zimnoch
Copy link
Member

lukasz-zimnoch commented Jun 20, 2023

It's only because of the --ignore-scripts flag that they are not being executed and we need to run a separate yarn run postinstall command

Nope :) the problem with bcrypto is more complicated. Specifically, because bcoin holds their node_modules in VCS (https://github.com/bcoin-org/bcoin/tree/master/node_modules). That means bcrypto dependency is already here when we run yarn install. For an unknown reason, yarn install does not build all native components in that case, hence the need for manual postinstall that does it again under the hood. The --ignore-scripts flag has basically nothing to do here.

@Shadowfiend
Copy link
Contributor

Idle observer here, but that info is captured neither in a code comment nor in the commit message that introduced the separate post install call (5fbdc20). Probably worth fixing that given the complexity of the reason :)

@michalinacienciala
Copy link
Contributor Author

Idle observer here, but that info is captured neither in a code comment nor in the commit message that introduced the separate post install call (5fbdc20). Probably worth fixing that given the complexity of the reason :)

FYI, I'm adding the explanation in #642.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:shipit: deployment Deployments and CI workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants