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

guix: build only supported targets using guix-start, update guix-start and guix-check to work correctly outside of containers #6390

Merged
merged 9 commits into from
Nov 14, 2024

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Nov 11, 2024

Issue being fixed or feature implemented

#6382 (comment) #6388 (comment)

alternative to #6388

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@UdjinM6 UdjinM6 added this to the 22 milestone Nov 11, 2024
kwvg
kwvg previously approved these changes Nov 11, 2024
Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

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

ACK 43e7985

knst
knst previously approved these changes Nov 12, 2024
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

LGTM a9c9faa

@UdjinM6
Copy link
Author

UdjinM6 commented Nov 12, 2024

Applied suggestions and also added 3ac5739 so local usage is now as simple as ./contrib/containers/guix/scripts/guix-start and ./contrib/containers/guix/scripts/guix-check, no extra args required.

@UdjinM6 UdjinM6 changed the title guix: build only supported targets using Guix container guix: build only supported targets using guix-start, update guix-start and guix-check to work correctly outside of containers Nov 12, 2024
Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

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

Running the script prints with an argument prints the value of ${1} before execution, this behaviour doesn't occur when it defaults to $(pwd)

$ ./contrib/containers/guix/scripts/guix-start
Found macOS SDK at '/src/dash/depends/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers', using...
Checking that we can connect to the guix-daemon...
^C
[...]

$ ./contrib/containers/guix/scripts/guix-start $(pwd)
/src/dash
Found macOS SDK at '/src/dash/depends/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers', using...
Checking that we can connect to the guix-daemon...
^C
[...]

if [[ ! -d "$WORKSPACE_PATH" ]]; then
echo "$0: $WORKSPACE_PATH is not a valid directory, exiting!"
if [[ ! -d "${WORKSPACE_PATH}" || ! "${WORKSPACE_PATH}" = /* || ! -f "${WORKSPACE_PATH}/contrib/guix/libexec/prelude.bash" ]]; then
echo "${0}: ${WORKSPACE_PATH} is not a valid directory, exiting!"
Copy link
Collaborator

@kwvg kwvg Nov 13, 2024

Choose a reason for hiding this comment

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

The message printed seems unwieldy, could be resolved by replacing ${0} with ${0##*/}

Before:

$ ./contrib/containers/guix/scripts/guix-start /
./contrib/containers/guix/scripts/guix-start: / is not a valid directory, exiting!

After:

./contrib/containers/guix/scripts/guix-start /
guix-start: / is not a valid directory, exiting!

Maybe error message should be tweaked? It is a valid directory, just the wrong directory.

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

LGTM c5d482e

maybe squash commits together?

Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

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

ACK c5d482e

@PastaPastaPasta PastaPastaPasta merged commit 9bfb700 into dashpay:develop Nov 14, 2024
35 checks passed
PastaPastaPasta added a commit that referenced this pull request Nov 26, 2024
…rocess.md`

87c31ad Update doc/release-process.md (UdjinM6)
55d7463 docs: mention building for some HOSTs only in `release-process.md` (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  dashpay/guix.sigs#73

  #6390 follow-up

  ## What was done?

  ## How Has This Been Tested?

  ## Breaking Changes

  ## Checklist:
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

Top commit has no ACKs.

Tree-SHA512: b4a2cadf5899a8aea6612b4ff9c0e9f9c530a9e2344eb090967fbcf9a2ab219aff02f11f86434e4082f84c401d578cf2d033b6838c94705f532beca4ab604986
knst pushed a commit to knst/dash that referenced this pull request Nov 26, 2024
…lease-process.md`

87c31ad Update doc/release-process.md (UdjinM6)
55d7463 docs: mention building for some HOSTs only in `release-process.md` (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  dashpay/guix.sigs#73

  dashpay#6390 follow-up

  ## What was done?

  ## How Has This Been Tested?

  ## Breaking Changes

  ## Checklist:
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

Top commit has no ACKs.

Tree-SHA512: b4a2cadf5899a8aea6612b4ff9c0e9f9c530a9e2344eb090967fbcf9a2ab219aff02f11f86434e4082f84c401d578cf2d033b6838c94705f532beca4ab604986
knst pushed a commit to knst/dash that referenced this pull request Nov 26, 2024
…lease-process.md`

87c31ad Update doc/release-process.md (UdjinM6)
55d7463 docs: mention building for some HOSTs only in `release-process.md` (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  dashpay/guix.sigs#73

  dashpay#6390 follow-up

  ## What was done?

  ## How Has This Been Tested?

  ## Breaking Changes

  ## Checklist:
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

Top commit has no ACKs.

Tree-SHA512: b4a2cadf5899a8aea6612b4ff9c0e9f9c530a9e2344eb090967fbcf9a2ab219aff02f11f86434e4082f84c401d578cf2d033b6838c94705f532beca4ab604986
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.

4 participants