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

Update Gambit, Gerbil, Glow #114449

Closed
wants to merge 17 commits into from
Closed

Update Gambit, Gerbil, Glow #114449

wants to merge 17 commits into from

Conversation

fare
Copy link
Contributor

@fare fare commented Feb 26, 2021

Update Gambit, Gerbil and Glow with Glow's first release v0.1.0.

Also include some changes in lib that Gerbil support relies on, notably including the POP object system, which by now is rather stable and proven for the purpose of building coherent locally modified sets of packages.

Motivation for this change
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.

@fare
Copy link
Contributor Author

fare commented Feb 26, 2021

@7c6f434c

@@ -0,0 +1,433 @@
# POP: Pure Object Prototypes
Copy link
Member

Choose a reason for hiding this comment

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

(There is a lot of preferring not to die on this hill in my position, and giving up on reasonably quick decision making in Nixpkgs, and giving up on consistency of decision making in Nixpkgs)

My impression is that a lot of extension mechanisms in Nixpkgs are like that because they try to pretend Nix is not a computationally-general-purpose programming language. POP has to come out and pop the bubble of pretence. Which is even a plus in my opinion, but raises the risks.

On the other hand, one extension mechanism that at least recognises its computational power is modules. Which are primarily used for NixOS but now sometimes also inside Nixpkgs. I think it would be nice to include them if you already compare to other extension mechanisms. Additionally, the thing that is popular to dislike about modules is their performance overhead. It's kind of worrying that the question of runtime CPU/RAM consumption is not prominently mentioned…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I admit I never hacked modules, so I cannot comment at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I created a PR just for POP #116275

@fare
Copy link
Contributor Author

fare commented Feb 27, 2021

I don't understand the failure in the grahamcofborg-eval job above.

@fare fare force-pushed the alpha branch 2 times, most recently from 3ee10fa to 5b8304d Compare March 4, 2021 18:20
pkgs/development/compilers/gerbil/glow-lang.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/gerbil/glow-lang.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/gerbil/glow-lang.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/gerbil/glow-lang.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/gerbil/glow-lang.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/gerbil/gerbil-utils.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/gerbil/gerbil-utils.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/gerbil/smug-gerbil.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
Copy link
Contributor Author

@fare fare left a comment

Choose a reason for hiding this comment

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

PTAL

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/gerbil/glow-lang.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/gerbil/smug-gerbil.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/gerbil/gerbil-utils.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/gerbil/gerbil-support.nix Outdated Show resolved Hide resolved
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

just a small nit. The lib is a bit out of my scope.

pkgs/development/compilers/gerbil/gerbil-ethereum.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/gerbil/glow-lang.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/gerbil/glow-lang.nix Outdated Show resolved Hide resolved
@fare
Copy link
Contributor Author

fare commented Mar 9, 2021

Did all the requested changes, and also merged in the changes from #113405.

PTAL.

@siraben
Copy link
Member

siraben commented Mar 9, 2021

@fare perhaps the POP-related stuff should be moved to a separate commit and possibly RFC? I'm not sure we need yet another extension mechanism (I'm not familiar with object systems). My preference would be to have this PR do the updates/refactoring with what we have currently and open another PR to add new functions to lib/attrsets.nix and the POP-related stuff.

Copy link
Member

@siraben siraben left a comment

Choose a reason for hiding this comment

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

After a first pass of pop.md, I like the idea, though I am one of those people who know pure lazy functional programming well and not object systems 😆. I definitely think it warrants further discussion outside of this Nixpkgs PR since if implemented could lead to wider changes and unification in the various heterogeneous extension mechanisms present in Nixpkgs.

lib/pop.md Outdated Show resolved Hide resolved
lib/pop.md Outdated Show resolved Hide resolved
lib/pop.md Outdated Show resolved Hide resolved
all their transitive dependencies, so as to manually compute and use
the precedence list, and maintain it as the code evolves.
This is a modularity disaster that prevents programmers from abstracting
over the details of which prototype requires what other prototype when used.
Copy link
Member

Choose a reason for hiding this comment

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

Related: shared flake overrides NixOS/nix#4004

overrides to a super object of type `B` sufficient to turn it into
an object of type `A`:

type Extension = A: B: Exists C: A B -> C | B // C <: A <: B;
Copy link
Member

@siraben siraben Mar 9, 2021

Choose a reason for hiding this comment

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

Perhaps explain a bit about the use of existential types here or use forall

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better?

and the `defaults` take precedence from left to right, then if not found
the `bottomDefaults` are consulted from right to left.

## TODO for POP
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to have in the RFC!

@fare
Copy link
Contributor Author

fare commented Mar 9, 2021

I could move the lib stuff to its own PR, but since it's actually of negative value unless used, that would be a catch-22.

Actually, I initially wanted to put pop inside gerbil... but then I found I couldn't extend gerbil with pop if pop was itself in gerbil (or at least it was trickier and uglier). So I moved it to lib, where it belongs eventually. I even started an RFC (82) about having an "official" way to deal with such "experimental" libraries, but it doesn't look like this will ever happen.

@fare fare force-pushed the alpha branch 2 times, most recently from 7f8970f to 3aa2b5f Compare March 10, 2021 18:07
@7c6f434c
Copy link
Member

@SuperSandro2000 should git-version be reported as possible typo when version is also there?

@SuperSandro2000
Copy link
Member

Can I insert a comment to disable the attribute-typo warning on line 25 of .../gambit/build.nix ? Other warnings were useful, and fixed.

No, nixpkgs-hammering does not have such a feature right now.

@SuperSandro2000 should git-version be reported as possible typo when version is also there?

probably not.

@@ -19,21 +20,22 @@ rec {
--replace "$(grep '^PACKAGE_VERSION=.*$' configure)" 'PACKAGE_VERSION="v${git-version}"' \
--replace "$(grep '^PACKAGE_STRING=.*$' configure)" 'PACKAGE_STRING="Gambit v${git-version}"' ;
'';
targets = "arm,java,js,php,python,riscv-32,riscv-64,ruby,x86,x86-64"; # eats 100% cpu on _digest
modules = false;
targets = "js"; # arm,java,js,php,python,riscv-32,riscv-64,ruby,x86,x86-64
Copy link
Member

Choose a reason for hiding this comment

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

Why removal of arm/x86-64 native targets? Purely for build time reasons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are various failures when building "modules" that don't have time to address. Also, while basically working, these backends are missing many features, and they are receiving to love at the moment. Hopefully at some point someone will make them really work, but until then, it's too much hassle for too little upside.

fare and others added 16 commits September 6, 2021 21:41
Like the many Jsonnet-style object systems already in Nix, pop combines
instance field values and composable prototype information in a same attrset.
However pop also implements CLOS-style DAG-based multiple inheritance.
Also use foldr when it makes sense.
Add an indirection between gerbil package parameters and nix packages
computed from those parameters, so overlays can easily override the former.

Use POP to override packages when the sha256 doesn't match.

Run unit-tests of gerbil packages (not integration tests).

Fix binaries for gerbil packages.

Fix support for gerbil packages including binaries.
@7c6f434c
Copy link
Member

but then I found I couldn't extend gerbil with pop if pop was itself in gerbil (or at least it was trickier and uglier).

I still don't get it (and we don't have enough shepherd volunteers to reopen global-POP RFC so it makes sense to revisit the least-impact approach).

Let POP live in gerbil directory tree, every version of Gerbil just imports POP and uses it, and then reexports it. If you need POP to extend Gerbil, you just grab it (and it doesn't matter where you grab it from, it is the same import)

I guess you could also have default.nix for Gerbil doing just general composition and defining attributes for various versions, then the top level would just inherit the attributes from it. Then POP can also be taken from gebrilVersions too, if desired.

@fare
Copy link
Contributor Author

fare commented Sep 10, 2021

As in put it in the passthru of some derivation? Would that cause the derivation to be built?
Or just register it under pkgs.gerbil-support.POP or something?

@7c6f434c
Copy link
Member

Nix is lazy, so passthru stuff can be used without building the derivation to which it is attached. gerbil-support makes sense, too, of course. I don't have strong opinions here, I just want to avoid global lib changes that are large enough to be controversial.

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.

5 participants