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

Use existing bash instance when running destroyScript #593

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

jmbaur
Copy link
Contributor

@jmbaur jmbaur commented Apr 5, 2024

The destroy script already has a bash interpreter in use, but when the disk-deactivate script is ran, it calls out to /usr/bin/env to query for a bash interpreter. When running the destroyScript in unique places (such as the stage-1 initrd), /usr/bin/env might not exist, so we can make destroyScript more self-contained by reusing the same bash.

@Lassulus
Copy link
Collaborator

Lassulus commented Apr 5, 2024

hmm, not sure how portable that is. but it works in the tests, so let's hope it doesn't break anyones usecase

@Lassulus
Copy link
Collaborator

Lassulus commented Apr 5, 2024

@mergify queue

Copy link
Contributor

mergify bot commented Apr 5, 2024

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@@ -449,7 +449,7 @@ let

# shellcheck disable=SC2043
for dev in ${toString (lib.catAttrs "device" (lib.attrValues devices.disk))}; do
${../disk-deactivate}/disk-deactivate "$dev"
$BASH ${../disk-deactivate}/disk-deactivate "$dev"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?

Suggested change
$BASH ${../disk-deactivate}/disk-deactivate "$dev"
${BASH:-} ${../disk-deactivate}/disk-deactivate "$dev"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied this change

@jmbaur jmbaur force-pushed the jbaur/shell-self-contained branch from 69821f5 to e3448ff Compare April 7, 2024 19:05
@Lassulus
Copy link
Collaborator

@mergify rebase

The destroy script already has a bash interpreter in use, but when the
`disk-deactivate` script is ran, it calls out to /usr/bin/env to query
for a bash interpreter. When running the destroyScript in unique places
(such as the stage-1 initrd), /usr/bin/env might not exist, so we can
make destroyScript more self-contained by reusing the same bash.
Copy link
Contributor

mergify bot commented Apr 12, 2024

rebase

✅ Branch has been successfully rebased

@Lassulus
Copy link
Collaborator

@mergify queue

Copy link
Contributor

mergify bot commented Apr 12, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 8d4ae69

@mergify mergify bot merged commit 8d4ae69 into nix-community:master Apr 12, 2024
41 checks passed
@jmbaur jmbaur deleted the jbaur/shell-self-contained branch April 12, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants