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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Allow setting web3signer version through environment #3368

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Allow setting web3signer version through environment #3368

wants to merge 1 commit into from

Conversation

philipmw
Copy link
Contributor

@philipmw philipmw commented Jul 23, 2022 •
edited
Loading

Issue Addressed

#3369

Proposed Changes

The goal is to make it possible to build Lighthouse without network access,
so builds can be reproducible.

This parallels the existing functionality in common/deposit_contract/build.rs,
which allows specifying a filename through the environment to avoid downloading
it. In this case, by specifying the version and making it available on the
filesystem, the existing logic will avoid a network download.

@CLAassistant
Copy link

CLAassistant commented Jul 23, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@michaelsproul michaelsproul changed the base branch from stable to unstable July 26, 2022 00:35
@michaelsproul michaelsproul added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Jul 26, 2022
@michaelsproul
Copy link
Member

Thanks for the PR @philipmw. I think we'll be able to merge this soon, but it needs a cargo-fmt fix first

@philipmw philipmw force-pushed the env-var branch 2 times, most recently from 3a46e84 to 481d4bd Compare July 26, 2022 02:22
@philipmw
Copy link
Contributor Author

Thanks for the review, @michaelsproul. I fixed the formatting and updated the request.

@michaelsproul michaelsproul added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jul 26, 2022
@philipmw
Copy link
Contributor Author

Hmm, I see my change is causing a CI failure. I am not familiar with Rust. I'll try to fix it, but meanwhile if you know how to solve this, I'd appreciate a tip.

@michaelsproul
Copy link
Member

michaelsproul commented Jul 26, 2022 •
edited
Loading

I think you should add the condition here:

lighthouse/testing/web3signer_tests/build.rs

Lines 30 to 32 in 21dec6f

let version = if let Some(version) = FIXED_VERSION_STRING {
version.to_string()
} else {

The env::var function can't be evaluated inside a const. You could add a branch after the FIXED_VERSION_STRING check like } else if let Ok(env_version) = env::var("LIGHTHOUSE_WEB3SIGNER_VERSION") { }. You'll also need to use OsString::into_string to convert to UTF-8 (you can call unwrap to panic on error, seeing as this is a build script).

@philipmw
Copy link
Contributor Author

Thanks for the tip! I rewrote it as you suggest.

@philipmw
Allow setting web3signer version through environment (sigp#3369)
cfdcc29 
The goal is to make it possible to build Lighthouse without network access,
so builds can be reproducible.

This parallels the existing functionality in `common/deposit_contract/build.rs`,
which allows specifying a filename through the environment to avoid downloading
it. In this case, by specifying the version and making it available on the
filesystem, the existing logic will avoid a network download.
@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jul 27, 2022
@michaelsproul
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jul 27, 2022
@philipmw
Allow setting web3signer version through environment (#3368)
cf3bcca 
## Issue Addressed

#3369 

## Proposed Changes

The goal is to make it possible to build Lighthouse without network access,
so builds can be reproducible.

This parallels the existing functionality in `common/deposit_contract/build.rs`,
which allows specifying a filename through the environment to avoid downloading
it. In this case, by specifying the version and making it available on the
filesystem, the existing logic will avoid a network download.
@bors
Copy link

bors bot commented Jul 27, 2022

Pull request successfully merged into unstable.

Build succeeded:

@bors bors bot changed the title Allow setting web3signer version through environment [Merged by Bors] - Allow setting web3signer version through environment Jul 27, 2022
@bors bors bot closed this Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelsproul michaelsproul michaelsproul approved these changes

Assignees
No one assigned
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@philipmw @CLAassistant @michaelsproul