| Message ID | 20200204125314.27494-1-ludo@gnu.org |
|---|---|
| Headers | show |
| Series | Clarify search path handling in commencement.scm | expand |
Ludovic Courtès writes: Hello Ludo', > The patch below for current ‘core-updates’ is an attempt to clarify > search path handling in commencement.scm by: (Weird, this initial mail got sorted into a new debbugs-submit folder, only found it later) > 1. Having ‘native-search-paths’ fields only for compilers—e.g., > “C_INCLUDE_PATH” belongs to the compiler, not to libc. > > 2. Avoiding phases that manually fiddle with search path > environment variables: normally, this is handled automatically > by the ‘set-paths’ phase based on the declared search paths, > so manual fiddling should be a last resort and it should be > well commented so we remember why it’s there. > > This is an attempt to reduce complexity and keep things declarative > as much as possible. I’ve tested it on top of > 46312064de8ae0cca531fddbc4b5ec8421e5d866 and “guix build coreutils” > succeeds. Very nice, thank you. I was happy to get it to build and did not imagine all this fiddling could actually be workarounds that could (should!) all be removed. Not only will this help readability and maintenance, it will make porting this to other distributions (NixOS) a lot easier too. > There’s another pattern that I found hard to follow that has to do > with the reuse of build phases. > > For example, there’s a build phase named ‘setenv’ (perhaps we should > find a more descriptive name :-)) in the various GCCs that is reused > or replaced; when looking at a specific package, it’s difficult to > see which phases it really runs because this particular phase is > inherited and modified on several layers. If I can make time for it, > I’ll see if I can come up with a proposal to clarify this, but at any > rate, it’s probably something to keep in mind for future changes. > > Thoughts? (I’m particularly interested in your feedback, janneke!) Yes, I agree. A first step could be to use better names and possibly split it up into serveral stages: set-configure-shells, set-cc-paths? Doing this will probably only need overriding the set-cc-paths. I'm not sure how to make the inherit+replace issue more obvious but it has been biting me and annoying me too. Maybe when we get into this replace trickery it is better to not reuse parent's stages at all > (arguments > (substitute-keyword-arguments (package-arguments gcc-core-mesboot) but fully rewrite (arguments ...)? I haven't looked into the consequences. In any case, with patch it has gotten a lot better already, thank you! janneke
Jan Nieuwenhuizen <janneke@gnu.org> skribis: >> 1. Having ‘native-search-paths’ fields only for compilers—e.g., >> “C_INCLUDE_PATH” belongs to the compiler, not to libc. >> >> 2. Avoiding phases that manually fiddle with search path >> environment variables: normally, this is handled automatically >> by the ‘set-paths’ phase based on the declared search paths, >> so manual fiddling should be a last resort and it should be >> well commented so we remember why it’s there. >> >> This is an attempt to reduce complexity and keep things declarative >> as much as possible. I’ve tested it on top of >> 46312064de8ae0cca531fddbc4b5ec8421e5d866 and “guix build coreutils” >> succeeds. > > Very nice, thank you. I was happy to get it to build and did not > imagine all this fiddling could actually be workarounds that could > (should!) all be removed. Not only will this help readability and > maintenance, it will make porting this to other distributions (NixOS) a > lot easier too. Yes, I hope so! >> There’s another pattern that I found hard to follow that has to do >> with the reuse of build phases. >> >> For example, there’s a build phase named ‘setenv’ (perhaps we should >> find a more descriptive name :-)) in the various GCCs that is reused >> or replaced; when looking at a specific package, it’s difficult to >> see which phases it really runs because this particular phase is >> inherited and modified on several layers. If I can make time for it, >> I’ll see if I can come up with a proposal to clarify this, but at any >> rate, it’s probably something to keep in mind for future changes. >> >> Thoughts? (I’m particularly interested in your feedback, janneke!) > > Yes, I agree. A first step could be to use better names and possibly > split it up into serveral stages: set-configure-shells, set-cc-paths? > > Doing this will probably only need overriding the set-cc-paths. I'm not > sure how to make the inherit+replace issue more obvious but it has > been biting me and annoying me too. Yes, I’m not sure exactly how to do it. > Maybe when we get into this replace trickery it is better to not reuse > parent's stages at all > >> (arguments >> (substitute-keyword-arguments (package-arguments gcc-core-mesboot) > > but fully rewrite (arguments ...)? Yes, either that or always inherit from the same package—e.g., always inherit from ‘gcc’ instead of inheriting from ‘gcc-mesboot1’, which inherits from ‘gcc-mesboot0’, and so on. That way one doesn’t have to walk the inheritance chain to understand what the phases are. Another option is to also make the interesting phases generic enough that we don’t have to modify them in package variants. We’ll see! Ludo’.
Hello Guix! The patch below for current ‘core-updates’ is an attempt to clarify search path handling in commencement.scm by: 1. Having ‘native-search-paths’ fields only for compilers—e.g., “C_INCLUDE_PATH” belongs to the compiler, not to libc. 2. Avoiding phases that manually fiddle with search path environment variables: normally, this is handled automatically by the ‘set-paths’ phase based on the declared search paths, so manual fiddling should be a last resort and it should be well commented so we remember why it’s there. This is an attempt to reduce complexity and keep things declarative as much as possible. I’ve tested it on top of 46312064de8ae0cca531fddbc4b5ec8421e5d866 and “guix build coreutils” succeeds. There’s another pattern that I found hard to follow that has to do with the reuse of build phases. For example, there’s a build phase named ‘setenv’ (perhaps we should find a more descriptive name :-)) in the various GCCs that is reused or replaced; when looking at a specific package, it’s difficult to see which phases it really runs because this particular phase is inherited and modified on several layers. If I can make time for it, I’ll see if I can come up with a proposal to clarify this, but at any rate, it’s probably something to keep in mind for future changes. Thoughts? (I’m particularly interested in your feedback, janneke!) Ludo’. Ludovic Courtès (2): gnu: commencement: Avoid hard-coded GCC version numbers. gnu: commencement: Rationalize search path handling. gnu/packages/commencement.scm | 201 ++++++++-------------------------- 1 file changed, 48 insertions(+), 153 deletions(-)