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

[RFC 0032] Phase running changes for better nix-shell use #32

Merged
merged 6 commits into from
Nov 19, 2020

Conversation

dezgeg
Copy link
Contributor

@dezgeg dezgeg commented Aug 29, 2018

Making some minor tweaks to how phases are run in the stdenv to improve the UX of nix-shell.

Rendered view: https://github.com/dezgeg/rfcs/blob/phase-changes/rfcs/0032-run-phase-changes-for-better-nix-shell-use.md

@dezgeg dezgeg changed the title [RFCxxxx] Phase running changes for better nix-shell use [RFC0032] Phase running changes for better nix-shell use Aug 29, 2018
@dezgeg dezgeg changed the title [RFC0032] Phase running changes for better nix-shell use [RFC 0032] Phase running changes for better nix-shell use Aug 29, 2018
@samueldr

This comment has been minimized.

@samueldr
Copy link
Member

given that the issue being fixed is a pure UX issue, not fixing it wouldn't prevent any other work from happening.

Good UX is important in everything. Anything that is clumsy will be fumbled with, and possibly be hated by the end-user, that's not a Nix-specific statement! Don't sell the desire for improvement short.


Is there anything preventing your runPhase alternative to be implemented right now? If it were implemented, (I think) it could be also made compatible with the implemented RFC. This would already be a net increase in UX over running eval .... The only drawback I see is that using runPhase would clobber the runPhase name if one would implement a runPhase as a phase.


As for the RFC itself, other than the generic baseline deprecation concerns, the idea seems sound enough. In reality the "new way" isn't that much of a hassle compared to the status quo. This mostly affects the implementation for a default phase; the user overriding a phase won't need to bother, right?.

As a new~ish user and developer running hooks also makes more sense than clobbering the pre parts by setting a custom phase. That is, in addition to making it easier to run phases properly.

@dezgeg
Copy link
Contributor Author

dezgeg commented Aug 30, 2018

Is there anything preventing your runPhase alternative to be implemented right now?

No, it could be added straight away. But I am kind of hopeful it won't be needed.

In reality the "new way" isn't that much of a hassle compared to the status quo. This mostly affects the implementation for a default phase; the user overriding a phase won't need to bother, right?.

Correct. You can see the changes needed in this commit: dezgeg/nixpkgs@87959f3

@timokau
Copy link
Member

timokau commented Aug 30, 2018

Good idea! I was annoyed by this multiple times and the downsides to his seem tiny.

As more future work (out of scope of this RFC, which is good as-is):

  1. make it easier to emulate the build process. For example:
    1. provide a command that will print out a sort of shell script that emulates what nix-build would do. That would eliminate the need to figure out which phases to run and in which order.
    2. provide something like progressUntil installPhase which will execute everything up to but not including installPhase
  2. add a debugging flag to nix-build (--shell-on-failure) that drops the user into a nix-shell in the current build directory if the build fails
  3. figure out a way to make installPhase work in a nix-shell, maybe by mounting some tmpdir to $out

Out of all of those 1.i. should be pretty easy to implement and a great help already.

@dezgeg
Copy link
Contributor Author

dezgeg commented Aug 30, 2018

As more future work (out of scope of this RFC, which is good as-is):

I actually experimented with implementing such tool at some point: https://github.com/dezgeg/nix-debug-shell but I am not particularly good with things like documentation or finishing stuff that I've started, so it's in a kind of forgotten state and potentially bitrotted by now. Maybe it could be moved to https://github.com/nix-community/ if there are people willing to work on it.

@Ericson2314
Copy link
Member

Nice proposal. I definitely want this.

https://github.com/ryantrinkle/nix-build-inplace is another such debug build helper.

@FRidh
Copy link
Member

FRidh commented Aug 30, 2018

For Python (mk-python-derivation.nix) we need this

    buildPhase = ''
        buildPhase
        (cd foo; buildPhase)
        (cd bar; buildPhase)
    '';

I'm glad to see this is covered.

This RFC looks good to me, though I think the Summary can be improved. Running the hooks independent of the phases has nothing to do with the UX of nix-shell usage and so that change should already be mentioned there.

@dezgeg
Copy link
Contributor Author

dezgeg commented Aug 30, 2018

For Python (mk-python-derivation.nix) we need this

@FRidh do you have a pointer on where is there is such code in Python infra that need changes? I have found (and fixed) some such cases in my branch but none in Python.

[...] I think the Summary can be improved. Running the hooks independent of the phases has nothing to do with the UX of nix-shell usage and so that change should already be mentioned there.

Yeah, the hook changes are an afterthought that came up after I started writing the description... I will reword it a bit.

@Ericson2314
Copy link
Member

Ericson2314 commented Aug 30, 2018

This is probably off topic, but I'd like if we made the findInputs dependency stuff in a phase, which like preHook is run by nix-shell by default for backwards compat. It seems more consistent to do all "real work" in phases.

@FRidh
Copy link
Member

FRidh commented Aug 31, 2018

@dezgeg fixupPhase in pkgs/development/interpreters/python/mk-python-derivation.nix


Actually, reading it again it is a bit different.

@dezgeg
Copy link
Contributor Author

dezgeg commented Aug 31, 2018

Actually, reading it again it is a bit different.

Yep, it's a different case. The sort of Nix-level code in mk-python-derivation.nix doesn't require any changes. Changes are only needed if inside a fixupPhase = '' ... ''; block (or equivalently, postFixup) there is a call to the Bash function fixupPhase.

@FRidh
Copy link
Member

FRidh commented Sep 1, 2018

@dezgeg do you have a branch with this change? I would like to try it out, especially because of NixOS/nixpkgs@9333c1b

@dezgeg
Copy link
Contributor Author

dezgeg commented Sep 1, 2018

The branch is mentioned under related-issues: https://github.com/dezgeg/nixpkgs/tree/better-run-phases

@timokau
Copy link
Member

timokau commented Oct 5, 2018

Since there is nobody to decide and there hasn't been any opposition, should we merge this?

@zimbatm
Copy link
Member

zimbatm commented Oct 5, 2018

It seems like a good target for 19.03. How much pain will it create for out-of-tree nix code?

@veprbl veprbl mentioned this pull request Oct 20, 2018
9 tasks
@lheckemann
Copy link
Member

I usually use runHook buildPhase, is there anything wrong with that?

@shlevy
Copy link
Member

shlevy commented Jan 17, 2019

@dezgeg Are you ready for us to open nominations for a shepherd team as per the new RFC process?

@shlevy
Copy link
Member

shlevy commented Jan 24, 2019

Closing for now. Feel free to reopen when this is ready for general review and shepherding.

@Mic92
Copy link
Member

Mic92 commented Jun 25, 2020

@lheckemann Do you still plan to make a meeting for this?

Ericson2314 referenced this pull request in NixOS/nix Aug 29, 2020
For example, for building the Nix flake, you would do:

  $ nix develop --configure
  $ nix develop --install
  $ nix develop --installcheck
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nixpkgs-phases-which-shell-language-how-to-overwrite-phases/9307/3

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/how-to-create-package-with-multiple-sources/9308/3

@lheckemann
Copy link
Member

So, I've finally arranged the meeting with @Ericson2314 and @samueldr. Results:

  • This RFC looks good generally
  • Unresolved question "This needs some testing to see if this works with the ancient Darwin Bash." — this should, as far as we know, not be a problem: the darwin bootstrap uses a more recent bash from the bootstrap tarball, not the macOS system bash; and nix-shell uses bashInteractive from <nixpkgs>, which is also recent enough for this not to be a concern.
  • Unresolved question regarding elegance of the way that backwards compatibility is ensured: we consider this an implementation detail which should not be an issue for the RFC in general.
  • Some general discussion on backwards-incompatible changes to stdenv. We decided not to bike-shed this RFC on that subject :)
  • We're not sure who will implement this. Both @dezgeg and @globin haven't been very active recently and may not have the capacity/time currently.
  • The last concern notwithstanding, we decided to call the Final Comment Period on this RFC, with disposition to accept. Speak within the next 10 days, or forever hold your peace!
  • Implementation need not follow immediately, but we believe accepting this RFC is helpful to indicate the community's general acceptance of the idea.

@lheckemann lheckemann added status: FCP in Final Comment Period and removed status: in discussion labels Nov 9, 2020
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/rfc32-phase-running-changes-for-better-nix-shell-use-fcp/9918/1

Comment on lines +64 to +90
-buildPhase() {
+defaultBuildPhase() {
- runHook preBuild
-
# set to empty if unset
: ${makeFlags=}

@@ -1008,14 +1104,14 @@ buildPhase() {
make ${makefile:+-f $makefile} "${flagsArray[@]}"
unset flagsArray
fi
-
- runHook postBuild
}
+
+buildPhase() {
+ runHook preBuild
+
+ if [ -n "$buildPhase" ]; then
+ eval "$buildPhase"
+ else
+ defaultBuildPhase
+ fi
+
+ runHook postBuild
+}

Copy link
Member

Choose a reason for hiding this comment

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

Has this not been implemented yet, or implemented differently?

Copy link
Member

Choose a reason for hiding this comment

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

Haven't seen @dezgeg in a while, don't think this ever got implemented.

Copy link
Member

Choose a reason for hiding this comment

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

Fwiw, NixOS/nixpkgs#175605 suggests a perhaps less breaking solution that's opt-in. Opting in is in itself messy though, so that's a reason to prefer this RFC if feasible.

infinisil referenced this pull request in nixpkgs-architecture/rfc-140 Jan 30, 2023
KAction pushed a commit to KAction/rfcs that referenced this pull request Apr 13, 2024
Co-authored-by: Linus Heckemann <git@sphalerite.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.