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

Refactor Raw pattern, part of #7479 #8839

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Aug 16, 2023

Motivation

Types converted:

  • NixStringContextElem
  • OutputsSpec
  • ExtendedOutputsSpec
  • DerivationOutput
  • DerivationType

Existing ones mostly conforming the pattern cleaned up:

  • ContentAddressMethod
  • ContentAddressWithReferences

The DerivationGoal::derivationType field had a bogus initialization, now caught, so I made it std::optional. I think #8829 can make it non-optional again because it will ensure we always have the derivation when we construct a DerivationGoal.

Context

See #7479 for details.

git grep 'Raw::Raw' indicates the two types I didn't yet convert DerivedPath and BuiltPath (and their Single variants) . This is because @roberth and I (can't find issue right now...) plan on reworking them somewhat, so I didn't want to churn them more just yet.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

@github-actions github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Aug 16, 2023
@roberth roberth changed the title Begin fixing issue #7479 Refactor Raw pattern, part of #7479 Aug 16, 2023
Copy link
Member

@edolstra edolstra left a comment

Choose a reason for hiding this comment

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

Looks good apart from the macOS build failure.

@@ -195,7 +195,7 @@ void LocalDerivationGoal::tryLocalBuild()
else if (settings.sandboxMode == smDisabled)
useChroot = false;
else if (settings.sandboxMode == smRelaxed)
useChroot = derivationType.isSandboxed() && !noChroot;
useChroot = derivationType->isSandboxed() && !noChroot;
Copy link
Member

Choose a reason for hiding this comment

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

Better to use derivationType().value() here, since it's not obvious whether derivationType is initialized. (An assertion failure would be better than an exception though...)

Copy link
Member Author

@Ericson2314 Ericson2314 Aug 18, 2023

Choose a reason for hiding this comment

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

Good idea, I just added one a few lines above.

src/libstore/derivations.hh Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 force-pushed the string-context-7479 branch 2 times, most recently from a6dbd66 to fedcd53 Compare August 18, 2023 15:42
Types converted:

- `NixStringContextElem`
- `OutputsSpec`
- `ExtendedOutputsSpec`
- `DerivationOutput`
- `DerivationType`

Existing ones mostly conforming the pattern cleaned up:

- `ContentAddressMethod`
- `ContentAddressWithReferences`

The `DerivationGoal::derivationType` field had a bogus initialization,
now caught, so I made it `std::optional`. I think NixOS#8829 can make it
non-optional again because it will ensure we always have the derivation
when we construct a `DerivationGoal`.

See that issue (NixOS#7479) for details on the general goal.

`git grep 'Raw::Raw'` indicates the two types I didn't yet convert
`DerivedPath` and `BuiltPath` (and their `Single` variants) . This is
because @roberth and I (can't find issue right now...) plan on reworking
them somewhat, so I didn't want to churn them more just yet.

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
@Ericson2314 Ericson2314 merged commit 665ad4f into NixOS:master Aug 18, 2023
@Ericson2314 Ericson2314 deleted the string-context-7479 branch August 18, 2023 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants