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

Factor out InstallableStorePath to its own file, dedup #7744

Merged
merged 1 commit into from
Feb 13, 2023

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Feb 3, 2023

Motivation

nix app had something called InstallableDerivedPath which is actually the same thing. We go with the later's name because it has become more correct.

Context

I originally did this change (more hurriedly) as part of #6225 --- a mini store-only Nix and a full Nix need to share this code. In the first RFC meeting for NixOS/rfcs#134 we discussed how some splitting of the massive installables.cc could begin prior, as that is a good thing anyways. (@edolstra's words, not mine!) This would be one such PR.

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
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or bug fix: updated release notes

`nix app` had something called `InstallableDerivedPath` which is
actually the same thing. We go with the later's name because it has
become more correct.

I originally did this change (more hurriedly) as part of NixOS#6225 --- a
mini store-only Nix and a full Nix need to share this code. In the first
RFC meeting for NixOS/rfcs#134 we discussed how
some splitting of the massive `installables.cc` could begin prior, as
that is a good thing anyways. (@edolstra's words, not mine!) This would
be one such PR.
@tomberek
Copy link
Contributor

tomberek commented Feb 12, 2023

Would be helpful to have a diagram or some description of the types/sub-types involved. (doxygen?) This might be out of scope for this change, but would be helpful overall.

  • Tests pass
  • dedup with InstallableDerivedPath and InstallableStorePath is good
  • no user-facing changes
  • refactor supports further efforts to impose layering

Copy link
Contributor

@tomberek tomberek left a comment

Choose a reason for hiding this comment

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

  • strictly an improvement
  • no regressions

@Ericson2314
Copy link
Member Author

Thank you @tomberek! Agree Doxygen would be good. I'll try to look into that this upcoming week.

@Ericson2314
Copy link
Member Author

#7814 issue for API docs.

@tomberek tomberek merged commit 601faa0 into NixOS:master Feb 13, 2023
@Ericson2314 Ericson2314 deleted the split-installable-store-path branch February 13, 2023 15:04
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-02-10-nix-team-meeting-minutes-31/25438/1

@Ericson2314 Ericson2314 mentioned this pull request Feb 20, 2023
10 tasks
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-team-report-2022-10-2023-03/27486/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants