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: init at 1.1.0 #85463

Closed
wants to merge 8 commits into from
Closed

guix: init at 1.1.0 #85463

wants to merge 8 commits into from

Conversation

@bqv
Copy link
Contributor

bqv commented Apr 17, 2020

Motivation for this change

Add Guix (migrated from #84004 because I'm a moron)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@bqv
Copy link
Contributor Author

bqv commented Apr 18, 2020

If you'd like to actually test this, I have a module-ish sufficient here that allows me to guix environment -C guix --ad-hoc bash vim guile-json (which fetched the relevant packages, grafted a few, built a profile and dropped me into a working shell)

@Emiller88
Copy link
Contributor

Emiller88 commented Apr 20, 2020

Just for future reference what does this have over #56430?

@bqv
Copy link
Contributor Author

bqv commented Apr 20, 2020

This is just the package without the module for now, builds a functional guix from source rather than using the guix binary bootstrap package, and isn't a year-old PR :p

@bqv bqv force-pushed the bqv:guix branch from 9278c0b to a96af1d May 4, 2020
@cole-h
Copy link
Member

cole-h commented May 4, 2020

@ofborg build guile-gcrypt guile-git guile-json guile-sqlite3 guile-ssh guix

@cole-h
cole-h approved these changes May 4, 2020
Copy link
Member

cole-h left a comment

Diff LGTM, guix --help displays a long list of subcommands, and borg is happy.

[16 built, 18 copied (53.8 MiB), 11.7 MiB DL]
https://github.com/NixOS/nixpkgs/pull/85463
6 packages built:
guile-gcrypt guile-git guile-json guile-sqlite3 guile-ssh guix
@nixos-discourse
Copy link

nixos-discourse commented May 4, 2020

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/147

Copy link
Member

timokau left a comment

Excited to see this merged!


postConfigure = ''
sed -i '/moddir\s*=/s%=.*%=''${out}/share/guile/site%' Makefile;
sed -i '/godir\s*=/s%=.*%=''${out}/share/guile/ccache%' Makefile;

This comment has been minimized.

Copy link
@timokau

timokau May 7, 2020

Member

Is it not possible to set these variables through environment variables or pass them to make somehow?

This comment has been minimized.

Copy link
@bqv

bqv May 9, 2020

Author Contributor

No (or I would have done :p)

I know this is flimsy, but it's also a very short Makefile so there's little room for error here.

pkgs/development/guile-modules/guile-git/default.nix Outdated Show resolved Hide resolved

postConfigure = ''
sed -i '/moddir\s*=/s%=.*%=''${out}/share/guile/site%' Makefile;
sed -i '/godir\s*=/s%=.*%=''${out}/share/guile/ccache%' Makefile;

This comment has been minimized.

Copy link
@timokau

timokau May 7, 2020

Member

Same comment from before applies here. If we need to do this multiple times, that's even more reason to find a better way.

This comment has been minimized.

Copy link
@bqv

bqv May 9, 2020

Author Contributor

I'm open to ideas, but it's hard coded in the Makefile. I don't see any other way beyond text manipulation

pkgs/development/guile-modules/guile-git/default.nix Outdated Show resolved Hide resolved
pkgs/tools/package-management/guix/default.nix Outdated Show resolved Hide resolved

GUILE_LOAD_PATH = let
guilePath = [
"\${out}/share/guile/site"

This comment has been minimized.

Copy link
@timokau

timokau May 7, 2020

Member

Won't this try to reference $out at runtime?

This comment has been minimized.

Copy link
@bqv

bqv May 9, 2020

Author Contributor

No, it falls through to be evaluated while creating the wrappers in installPhase. There may be a better way of quoting them, though, I'm not sure

This comment has been minimized.

Copy link
@timokau

timokau May 10, 2020

Member

Maybe placeholder "out" would be better. At least that way its more obvious.

};

meta = with lib; {
description = "Functional package manager for installed software packages and versions";

This comment has been minimized.

Copy link
@timokau

timokau May 7, 2020

Member

Is that an official tagline? The "installed" part doesn't make much sense to me.

This comment has been minimized.

Copy link
@bqv

bqv May 9, 2020

Author Contributor

It was, it's since changed...

This comment has been minimized.

Copy link
@bqv

bqv May 9, 2020

Author Contributor

Updated to something more sensical :)

@bqv bqv force-pushed the bqv:guix branch 4 times, most recently from 4d8ad12 to cd4c840 May 9, 2020
@bqv bqv force-pushed the bqv:guix branch from cd4c840 to 2a760f6 May 9, 2020
};

postConfigure = ''
sed -i '/moddir\s*=/s%=.*%=''${out}/share/guile/site%' Makefile;

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 9, 2020

Contributor

Did you try something like this:

preConfigure = ''
configureFlags="$configureFlags --with-guilesitedir=$out/share/guile/site"
'';

Or perhaps even better:

configureFlags = [
  "--with-guilesitedir=${placeholder "out"}/share/guile/site"
];

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 9, 2020

Contributor

If that doesn't work I'd advice contacting upstream.

This comment has been minimized.

Copy link
@bqv

bqv May 9, 2020

Author Contributor

Yes, like I said to timo above, it's not something that can be substituted by autotools, it's hardcoded in the Makefile.

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 9, 2020

Contributor

And what's wrong with this output:

$ tree $(nix-build -A guile-json)
result
├── lib
│   └── guile
│       └── 2.2
│           └── site-ccache
│               ├── json
│               │   ├── builder.go
│               │   └── parser.go
│               └── json.go
└── share
    └── guile
        └── site
            └── 2.2
                ├── json
                │   ├── builder.scm
                │   └── parser.scm
                └── json.scm

10 directories, 6 files

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 9, 2020

Contributor

This is what you get if you don't specify any configure flags nor apply that postConfigure hook.

This comment has been minimized.

Copy link
@bqv

bqv May 9, 2020

Author Contributor

These paths have to be linked at by guile, for every package needed by guix. Note that some of these packages by default do not have 2.2 in that path, some don't even properly specify their paths (and therefore end up trying to write directly to the guile derivation), or have extensions and try and write those somewhere that they won't be found. For the sake of consistency across guile modules and sanity of everyone trying to build this, I thought it might be nice to fix those

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 9, 2020

Contributor

These paths have to be linked at by guile

Are you talking about the wrapping done at guix/default.nix that adds GUILE_LOAD_COMPILED_PATH to the environment?

It just seems weird to me, that a package maintained by Guix's developers (at least in a certain sense) - a package that's needed to bootstrap a package manager which is based on Nix, will need such hacky workarounds to fix paths in $out. Guix doesn't put these to any of those guile dependencies, see:

Moreover, they have Guile 3.0.

@bqv bqv force-pushed the bqv:guix branch from 2a760f6 to 988da31 May 9, 2020
@bqv bqv force-pushed the bqv:guix branch from 988da31 to ea1769e May 9, 2020
@bqv bqv requested a review from timokau May 10, 2020
@bqv
Copy link
Contributor Author

bqv commented May 10, 2020

This is exhausting. I'm moving this to my local repository.

I'm going to close this because I'm not a fan of having ancient PRs open on my account which seem to have no hope of being merged.

For future reference, if anyone would like to use this, as of this head it is quite functional and I'm actually using it on my local machine. It is implemented in the sanest way I could think of, but if anyone thinks they can do better then please, please do.

@bqv bqv closed this May 10, 2020
@timokau
Copy link
Member

timokau commented May 10, 2020

Sad to hear that, thank you for your work so far. If someone wants to pick this up where @bqv left off, feel free to ping me in the new PR if you want a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.