-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
bluespec: init at unstable-2020.02.07 #79468
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
Conversation
@GrahamcOfBorg build bluespec-bsc |
Can we just take the opportunity to name the derivation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basic nits
}: | ||
|
||
let | ||
gmp-static = gmp.override { withStatic = true; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need static gmp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yices asks for it during configure phase. will add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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>
@GrahamcOfBorg build bluespec |
1 similar comment
@GrahamcOfBorg build bluespec |
This is awesome, thanks @flokli! |
710e1e9
to
1008dcf
Compare
@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 Having it in the 20.03 release would probably be really nice, and happy to backport bumps and fixes there as well :-) |
38a1fdd
to
cf4d17d
Compare
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I proposed this to upstream already in B-Lang-org/bsc#13.
Meant to say this in the review but, beyond that version number nit, this LGTM! |
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. |
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
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)cc @quark17 @ProfFan @arjenroodselaar @bpfoley