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

bluespec: init at unstable-2020.02.07 #79468

Merged
merged 1 commit into from Feb 10, 2020
Merged

bluespec: init at unstable-2020.02.07 #79468

merged 1 commit into from Feb 10, 2020

Conversation

@flokli
Copy link
Contributor

@flokli flokli commented Feb 7, 2020

bluespec open sourced their Bluespec compiler.

This is a WIP PR, as some patches are still needed to get it working and there hasn't been an official release yet.

There's only one single patch left, which removes a backend that upstream also is fine to stub out.

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.

cc @quark17 @ProfFan @arjenroodselaar @bpfoley

@flokli
Copy link
Contributor Author

@flokli flokli commented Feb 7, 2020

@GrahamcOfBorg build bluespec-bsc

@flokli flokli force-pushed the flokli:bluespec-bsc branch from 56b1225 to 7543f45 Feb 7, 2020
@flokli flokli changed the title WIP: bluespec-bsc: init at unstable-2020.02.05 WIP: bluespec-bsc: init at unstable-2020.02.07 Feb 7, 2020
@flokli flokli force-pushed the flokli:bluespec-bsc branch from 7543f45 to ad8f3be Feb 7, 2020
@thoughtpolice
Copy link
Member

@thoughtpolice thoughtpolice commented Feb 7, 2020

Can we just take the opportunity to name the derivation bluespec across the board? I hate being super picky, but I think it's simply a better name (bluespec-bsc = "the bluespec bluespec compiler"?) for the expression in general, while bsc is too short.

Copy link
Member

@thoughtpolice thoughtpolice left a comment

basic nits

pkgs/development/compilers/bluespec-bsc/default.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/bluespec-bsc/default.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/bluespec-bsc/default.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/bluespec-bsc/default.nix Outdated Show resolved Hide resolved
}:

let
gmp-static = gmp.override { withStatic = true; };

This comment has been minimized.

@thoughtpolice

thoughtpolice Feb 7, 2020
Member

Why do we need static gmp?

This comment has been minimized.

@flokli

flokli Feb 8, 2020
Author Contributor

yices asks for it during configure phase. will add a comment.

This comment has been minimized.

@thoughtpolice

thoughtpolice Feb 8, 2020
Member

I feel like we should fix that too, though. Like why does yices need a static GMP? That seems very suspicious; I guess they could keep it as a completely internal to their .so (and none of the symbols get exposed by the linker)... Strange.

This comment has been minimized.

@flokli

flokli Feb 8, 2020
Author Contributor

yices2 in nixpkgs also uses a static gmp, so that would be something to fix on that upstream.

I'd really much prefer if we could as the yices people for a release, remove the custom vendored version from the bsc codebase, and pass in our (bumped) nixpkgs version through pkgconfig.

(Or use something like SMT-lib to avoid linking alltogether, not sure)

For the scope of the initial PR, this is probably fine.

pkgs/development/compilers/bluespec-bsc/default.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/bluespec-bsc/default.nix Outdated Show resolved Hide resolved
thoughtpolice added a commit to thoughtpolice/yosys-bsv that referenced this pull request Feb 8, 2020
This will be used for CI/integration testing, shortly.

Note that Bluespec isn't yet in upstream Nixpkgs, but it will be soon
(NixOS/nixpkgs#79468).

Signed-off-by: Austin Seipp <aseipp@pobox.com>
@flokli flokli force-pushed the flokli:bluespec-bsc branch from ad8f3be to bce456a Feb 8, 2020
@ofborg ofborg bot requested a review from thoughtpolice Feb 8, 2020
@flokli flokli changed the title WIP: bluespec-bsc: init at unstable-2020.02.07 WIP: bluespec: init at unstable-2020.02.07 Feb 8, 2020
@flokli
Copy link
Contributor Author

@flokli flokli commented Feb 8, 2020

@GrahamcOfBorg build bluespec

1 similar comment
@flokli
Copy link
Contributor Author

@flokli flokli commented Feb 8, 2020

@GrahamcOfBorg build bluespec

@jcumming
Copy link

@jcumming jcumming commented Feb 8, 2020

This is awesome, thanks @flokli!

@flokli flokli force-pushed the flokli:bluespec-bsc branch from 5600c82 to 50530b4 Feb 8, 2020
@flokli flokli force-pushed the flokli:bluespec-bsc branch 2 times, most recently from 710e1e9 to 1008dcf Feb 8, 2020
@flokli
Copy link
Contributor Author

@flokli flokli commented Feb 8, 2020

@jcumming thanks for the good vibes, but also a huge thanks to @thoughtpolice and everybody else filing PRs to the upstream repo to get it to build, and to the bluespec people to review and merge these :-)

@thoughtpolice With your remarks addressed (except for the static gmp, which is also the case for our nixpkgs yices derivation) - what do you think about merging this?

Having it in the 20.03 release would probably be really nice, and happy to backport bumps and fixes there as well :-)

@flokli flokli force-pushed the flokli:bluespec-bsc branch 2 times, most recently from 38a1fdd to cf4d17d Feb 8, 2020
@flokli flokli force-pushed the flokli:bluespec-bsc branch from cf4d17d to d72b72d Feb 9, 2020
@flokli flokli force-pushed the flokli:bluespec-bsc branch from d72b72d to c9ea6af Feb 9, 2020
ghcWithPackages = haskell.packages.ghc844.ghc.withPackages (g: (with g; [old-time regex-compat syb]));
in stdenv.mkDerivation rec {
pname = "bluespec";
version = "unstable-2020.02.09";

This comment has been minimized.

@thoughtpolice

thoughtpolice Feb 10, 2020
Member

I don't think this is a good version string, because even if lib.compareVersion works against it, many tools would generally expect version identifiers to begin numerically. I do, at least.

Here's my suggestion: rather than inventing this, let's just bite the bullet and follow something close to calver/semver for this. Historically, Bluespec compiler releases were tagged as YEAR.MONTH e.g. 2014.08, which looks fine. So then let's extend this to support the metadata/prerelease notions of that:

version = "2020.02-unstable+g{builtins.substr 0 8 src.rev}

Where -unstable signifies the prerelease identifier (it might one day be -beta2 or -rc3 for example), and +g$HASH signifies the build metadata containing the git version, like in semver. But the basic major version numer is still calendar versioning.

In fact I normally even use a slightly augmented version of this, inspired by Nix itself, which tracks the number of commits to the head that you target. So it would actually, in this case, be:

vcsRev = "68"; # 68 commits as of 05c8afb08078e437
version = "2020.02-unstable+${vcsRev}-g{builtins.substr 0 8 src.rev}

Which would look something like bluespec-2020.02-unstable+68-g05c8afb0 which is wordy, but very accurate. (The 68 can be generated via git log --format=oneline | wc -l)

In the event of a stable release, we'd just change it to e.g. version = "2020.03" or something.

Does this sound reasonable?

This comment has been minimized.

@flokli

flokli Feb 10, 2020
Author Contributor

I'm just following https://github.com/NixOS/nixpkgs/blob/master/doc/contributing/coding-conventions.xml#L232 here, and don't see why we should do any different here.

Apart from that, I personally really like just using git describe --tags for version strings - but this would require upstream tag at least a single commit with something like 2020.02.

This comment has been minimized.

@flokli

flokli Feb 10, 2020
Author Contributor

I proposed this to upstream already in B-Lang-org/bsc#13.

@thoughtpolice
Copy link
Member

@thoughtpolice thoughtpolice commented Feb 10, 2020

Meant to say this in the review but, beyond that version number nit, this LGTM!

@ofborg ofborg bot requested a review from thoughtpolice Feb 10, 2020
@flokli
Copy link
Contributor Author

@flokli flokli commented Feb 10, 2020

As explained in the thread, the versioning follows https://github.com/NixOS/nixpkgs/blob/master/doc/contributing/coding-conventions.xml#L232, so let's keep this until there's a proper tag/release.

@flokli flokli changed the title WIP: bluespec: init at unstable-2020.02.07 bluespec: init at unstable-2020.02.07 Feb 10, 2020
@flokli flokli merged commit 0e9d542 into NixOS:master Feb 10, 2020
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@flokli flokli deleted the flokli:bluespec-bsc branch Feb 10, 2020
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

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