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

Enhanced support for Gerbil, Gambit #86843

Merged
merged 6 commits into from May 17, 2020
Merged

Enhanced support for Gerbil, Gambit #86843

merged 6 commits into from May 17, 2020

Conversation

fare
Copy link
Contributor

@fare fare commented May 4, 2020

Motivation for this change

As Gerbil releases its v0.16, I have not only tweaked and cleaned up Gerbil support in Nix, but also added support for Gerbil libraries that may depend on each other, including one seed example, gerbil-utils.

Questions to reviewers:

  • Am I doing the right thing when registering the packages in all-packages? Should the gerbilPackages be defined in their own file rather than in gerbil-support.nix ?
  • For gerbil libraries, at the moment, I only intend to import "unstable" versions from git, to work with gerbil-unstable. Instead of gerbilPackages.gerbil-utils-unstable, should it be gerbilPackage_unstable.gerbil-utils or something?
  • If I want to later provide some kind of wrapper in NixOS that export a GERBIL_LOADPATH and/or NIX_GERBIL_LOADPATH for all configured libraries, how do I do that? What about registering libraries for those in ~/.nix-profile vs /nix/var/nix/profiles vs /run/current-system/sw ? Just export GERBIL_LOADPATH=~/.nix-profile/gerbil/lib:/nix/var/nix/profiles/gerbil/lib:/run/current-system/sw/gerbil/lib ? Should I create an option to enable this environment variable?
  • Do I really need to put the version updates in a different commit than the gerbil-utils creation? Since the gerbil-utils creation is linked to all the nix code changes, is it OK if I put that one with them, and otherwise put the v0.16 update in its own earlier PR?

Notes:

  • Gerbil stable v0.16 is not released quite yet, but will be in the next few days. Gerbil stable will depend on Gambit stable, Gerbil unstable will depend on Gambit unstable, even though in practice it will be the very same code, with only the version number difference.
  • I will remove the [WIP] from this PR when Gerbil stable is out and reviewers have rubberstamped the general design, if not the actual code.
  • At the same time, once the proper way to do it is agreed upon, I may add a dozen more gerbil libraries, by automating an import from the gerbil directory.
Things done
  • Update gambit-unstable, gerbil-unstable. Create gerbil-utils.
  • Move the Gambit files from $out/ to $out/gambit/ which makes the profile much cleaner
  • Move the Gerbil files from $out/ to $out/gambit/ which makes the profile much cleaner
  • Add support for Gerbil libraries, that may depend on other libraries (tested with an overlay).
  • Create gerbil-support.nix for that.
  • Return to -O1 by default, but make that easily overridable.
  • 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.

@fare
Copy link
Contributor Author

fare commented May 4, 2020

@@ -1,34 +1,51 @@
{ stdenv, makeStaticLibraries,
{ gccStdenv, lib, makeStaticLibraries, callPackage,
Copy link
Member

Choose a reason for hiding this comment

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

Is callPackage needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not anymore. I realize that makeStaticLibraries neither.

@fare
Copy link
Contributor Author

fare commented May 6, 2020

PTAL. And please comment on the questions at top of PR.

@ofborg ofborg bot requested a review from 7c6f434c May 6, 2020 13:48
@7c6f434c
Copy link
Member

7c6f434c commented May 6, 2020

Am I doing the right thing when registering the packages in all-packages?

Mostly. I doubt it makes sense to give a top-level name to gerbilPackage function.

Should the gerbilPackages be defined in their own file rather than in gerbil-support.nix ?

i think it doesn't matter at that size. With more packages new considerations might arise.

For gerbil libraries, at the moment, I only intend to import "unstable" versions from git, to work with gerbil-unstable. Instead of gerbilPackages.gerbil-utils-unstable, should it be gerbilPackage_unstable.gerbil-utils or something?

In principle, the first sounds like «unstable library code» and the second like «stable library code running on unstable Gerbil». Not sure which is a closer description of the situation.

Is there a plan for stable-Gerbil-using library packages in the foreseeable future?

If I want to later provide some kind of wrapper in NixOS that export a GERBIL_LOADPATH and/or NIX_GERBIL_LOADPATH for all configured libraries, how do I do that? What about registering libraries for those in ~/.nix-profile vs /nix/var/nix/profiles vs /run/current-system/sw ? Just export GERBIL_LOADPATH=~/.nix-profile/gerbil/lib:/nix/var/nix/profiles/gerbil/lib:/run/current-system/sw/gerbil/lib ? Should I create an option to enable this environment variable?

Hmmm. I have a feeling that we generally claim that installing libraries into profiles does not work and it is not expected to work, one uses nix-shell (that runs setup-hook). So you probably need to define setup-hook that adds the Gerbil libraries from the build inputs to the environment variables (which is also needed for Gerbil packages to depend on each other). Then the recommended way of using libraries in an interactive shell would be nix-shell.

As someone who has stopped using NixOS mainline (and never used nix-env -iA much), I would prefer not to merge a NixOS change going against the recommended practices as I understand them.

Do I really need to put the version updates in a different commit than the gerbil-utils creation? Since the gerbil-utils creation is linked to all the nix code changes, is it OK if I put that one with them, and otherwise put the v0.16 update in its own earlier PR?

I think updating + creating the infrastructure in one commit, then adding the utils package in the next one is the closest to what people consider best practices here. In this specific case I don't care enough to actually enforce that it should be more than one commit.

@fare
Copy link
Contributor Author

fare commented May 7, 2020

Thanks a lot for the guidance.

In principle, the first sounds like «unstable library code» and the second like «stable library code running on unstable Gerbil». Not sure which is a closer description of the situation.
Is there a plan for stable-Gerbil-using library packages in the foreseeable future?

In the near future, all gerbil library packages will be unstable libraries running on top of unstable gerbil.

In some further future, there might be stable libraries on stable gerbil vs unstable libraries on unstable gerbil, and in a further further future, stable on unstable and unstable on stable. But that's all speculation.

@fare fare force-pushed the fare branch 3 times, most recently from 4d23898 to 0fbfc3d Compare May 7, 2020 09:31
@7c6f434c
Copy link
Member

7c6f434c commented May 7, 2020

Is it a good idea to recurseIntoAttrs then? I would consider just having gerbil-support-unstable at the top level, that happens to include gerbilPackages

@fare
Copy link
Contributor Author

fare commented May 7, 2020

The point of recurseIntoAttrs is to make the gerbil libraries visible to nix-env -qa.

@7c6f434c
Copy link
Member

7c6f434c commented May 7, 2020

I understand that, but on one hand they are purely unstable and on the other hand nix-env -qa is slowly getting more and more recommended against as fantastically inefficient and on the third hand nix-env -iA of libraries is something that is widely expected not to work

@ofborg ofborg bot requested a review from 7c6f434c May 9, 2020 02:57
Copy link
Member

@7c6f434c 7c6f434c left a comment

Choose a reason for hiding this comment

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

LGTM

fare added 6 commits May 17, 2020 15:48
Refactor the build rule:
- Put files in $out/gambit instead of $out.
- Make the optimization setting easy to override.
- Make use of gccStdenv more explicit at this level.
- Support new-style runtime options for forcing UTF-8 I/O.
- Override the PACKAGE_VERSION and PACKAGE_STRING with git version.
- Note that the license is lgpl21, not lpgl2 (Note: also dual asl20).
- Try and fail to meaningfully add missing runtimeDeps.
- Build using NIX_BUILD_CORES.
- Use the new Gambit support.
- Move files from $out to $out/gerbil.
- Use new Gerbil configuration and installation scripts.
- Move some fixups from preBuild to postPatch.
- Give up on previous failed attempts at using static libraries.
- Add support for compiling libraries written in Gerbil.
- Build using NIX_BUILD_CORES.
- Register all those things in all-packages.
Now that v0.16 was released at last, make the configurePhase and instalPhase
the same again for default and unstable.
@fare fare changed the title [WIP] Enhanced support for Gerbil, Gambit Enhanced support for Gerbil, Gambit May 17, 2020
@fare fare marked this pull request as ready for review May 17, 2020 20:23
@fare
Copy link
Contributor Author

fare commented May 17, 2020

PTAL. Gerbil v0.16 was released, everything is now ready for final review and merge.

@7c6f434c 7c6f434c merged commit 4961e59 into NixOS:master May 17, 2020
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.

None yet

2 participants