-
Notifications
You must be signed in to change notification settings - Fork 217
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
Fix restore bench and docker #2045
Conversation
c875084
to
edbcefc
Compare
edbcefc
to
13c55e0
Compare
nix/docker.nix
Outdated
exec ${exe}/bin/cardano-wallet-${backend} "$@" | ||
''; | ||
export LOCALE_ARCHIVE="${glibcLocales}/lib/locale/locale-archive" | ||
exec ${exe}/bin/${walletExeName} "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's a bash script, you could "cheat" and use a glob.
exec ${exe}/bin/cardano-wallet* "$@"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, that would break if we for some reason would end up with multiple matches?
If the cost is the two lines:
suffix = if backend == "shelley" then "" else "-" + backend;
walletExeName = "cardano-wallet" + suffix;
then maybe being explicit is worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The glob will be safe to use because this is the bin directory within a backend-specific package. But explicit filenames are probably easier to maintain.
nix/docker.nix
Outdated
let | ||
suffix = if backend == "shelley" then "" else "-" + backend; | ||
walletExeName = "cardano-wallet" + suffix; | ||
in writeScriptBin "start-cardano-wallet-${backend}" '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of this start script actually doesn't matter. It could be called globbits and the image would still work.
So you can remove the backend from its name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This I noticed, but in-doubt kept the name as before.
start-cardano-wallet
could make you believe it was shelley specific, even when the backend is jormungandr
.
Maybe just start
or start-wallet
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with start-wallet
now.
0244e35
to
d718f30
Compare
d718f30
to
783f6f1
Compare
Looks good, works for me. bors r+ |
2045: Fix restore bench and docker r=rvl a=Anviking # Issue Number #1919, #2044 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Fix broken restore bench https://buildkite.com/input-output-hk/cardano-wallet-nightly/builds/608#f02f336b-c7be-4b3f-b86b-bc825c9b5ac2 - [ ] _Hope_ `docker` should be fixed, but can't easily test locally⚠️ # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Johannes Lund <[email protected]>
Build failed
Retry:
|
bors r+ |
2045: Fix restore bench and docker r=Anviking a=Anviking # Issue Number #1919, #2044 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Fix broken restore bench https://buildkite.com/input-output-hk/cardano-wallet-nightly/builds/608#f02f336b-c7be-4b3f-b86b-bc825c9b5ac2 - [ ] _Hope_ `docker` should be fixed, but can't easily test locally⚠️ # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Johannes Lund <[email protected]>
Timed out |
bors r+ |
2045: Fix restore bench and docker r=Anviking a=Anviking # Issue Number #1919, #2044 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Fix broken restore bench https://buildkite.com/input-output-hk/cardano-wallet-nightly/builds/608#f02f336b-c7be-4b3f-b86b-bc825c9b5ac2 - [ ] _Hope_ `docker` should be fixed, but can't easily test locally⚠️ # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Johannes Lund <[email protected]>
Build failed
|
bors r+ |
2045: Fix restore bench and docker r=Anviking a=Anviking # Issue Number #1919, #2044 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Fix broken restore bench https://buildkite.com/input-output-hk/cardano-wallet-nightly/builds/608#f02f336b-c7be-4b3f-b86b-bc825c9b5ac2 - [ ] _Hope_ `docker` should be fixed, but can't easily test locally⚠️ # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Johannes Lund <[email protected]>
Build failed
|
bors r+ |
Build succeeded |
Issue Number
#1919, #2044
Overview
docker
should be fixed, but can't easily test locallyComments