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

bs-platform: 6.2.1 -> 7.0.1 #76104

Merged
merged 1 commit into from Jan 14, 2020
Merged

bs-platform: 6.2.1 -> 7.0.1 #76104

merged 1 commit into from Jan 14, 2020

Conversation

@turboMaCk
Copy link
Member

@turboMaCk turboMaCk commented Dec 20, 2019

see updates

Introduce buckeScriptPackages which makes version 6 still available
via buckleScriptPackages.bs-platform-6.
BuckleScript compiler is very picky about versions and performs checks
of versions before compilations therefore it's probably a good idea
to support at least few major versions of compiler.

Additional notes (not part of commit message):

  • Anouncment of release is few weeks old now.
  • I'm trying to follow the naming convetions from package sets I'm most familiar with like nodePackages, haskellPackages and elmPackages.
  • So far I'm thinking about supporting somewhere around 3 last major releases with one default bs-platform which is now the latest.
  • Build of 6 and 7 is practically identical - I'm not abstracting this just yet because it might not be future proof.

Checklist

  • 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 nix-review --run "nix-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.
Notify maintainers

cc @

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Jan 3, 2020

@jonringer @FRidh sorry for ping but since you two reviewed the last PR and this has no other reviewers for 2 weeks it would be great if you can review this.

@turboMaCk turboMaCk force-pushed the turboMaCk:bs-add-7 branch from ded24b4 to f9f6573 Jan 6, 2020
@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Jan 6, 2020

rebased

@turboMaCk turboMaCk force-pushed the turboMaCk:bs-add-7 branch from f9f6573 to cb4d1bd Jan 6, 2020
@yorickvP
Copy link
Contributor

@yorickvP yorickvP commented Jan 6, 2020

@GrahamcOfBorg build bs-platform

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Jan 6, 2020

Seems there is build failure on ARM. I think we might want to restrict this to intel though I'm not sure if bs suppose to support ARM or not.

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Jan 6, 2020

@GrahamcOfBorg build buckeScriptPackages.bs-platform-6

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Jan 6, 2020

@yorickvP may I ask you to try to build buckeScriptPackages.bs-platform-6 aarch64? It seems I'm not trusted user. Previous ARM build passed so I would like to confirm that this issue is specific to 7.0.1.

@yorickvP
Copy link
Contributor

@yorickvP yorickvP commented Jan 10, 2020

@GrahamcOfBorg build buckeScriptPackages.bs-platform-6

@anmonteiro
Copy link
Member

@anmonteiro anmonteiro commented Jan 11, 2020

Hey @turboMaCk, thanks for doing this work! It's really exciting to have BuckleScript in nixpkgs in a way that leverages the other packages of the nix ecosystem.

One thing I'm wondering but I'm unsure how to accomplish is: it would be awesome if we could override the src for the BuckleScript derivation in a way that would cause the OCaml one to be overridden too.

Right now, the way the derivation is written, I don't think it's possible to override BuckleScript and OCaml in tandem.

@anmonteiro
Copy link
Member

@anmonteiro anmonteiro commented Jan 11, 2020

For the record, and expanding a little on the above, I'm suggesting something like:

stdenv.mkDerivation rec {
  pname = "bs-platform";
  version = "7.0.1";

  src = fetchFromGitHub {
    owner = "BuckleScript";
    repo = "bucklescript";
    rev = "52770839e293ade2bcf187f2639000ca0a9a1d46";
    sha256 = "0s7g2zfhshsilv9zyp0246bypg34d294z27alpwz03ws9608yr7k";
    fetchSubmodules = true;
  };

  patchPhase =
    let
      ocaml = import ./ocaml.nix {
        bs-version = version;
        version = ocaml-version;
        inherit stdenv;
        # This uses a `src` which can be overridden with `overrideAttrs`
        src = "${src}/ocaml";
      };
    in
    "  ...  ";
}
@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Jan 11, 2020

@anmonteiro we can for sure do that. That's being said we still need to do something about aarch64. There is another thing I wonder if we would be able to resolve and that is usage of bs in projects within node_packages https://github.com/turboMaCk/bs-platform.nix#nix-shell

@turboMaCk turboMaCk force-pushed the turboMaCk:bs-add-7 branch from cb4d1bd to 6570174 Jan 11, 2020
@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Jan 11, 2020

@anmonteiro check the new diff. Now there is an abstraction over the build of BS platform which is reused between 6xx and 7xx version. I originally didin't want to do it because a lot of assumptions about the build has to be made and I feel we might expect there will be some significant changes between current builds and builds for future versions. Anyway in light of making it possible to override the compiler version I think it makes sense to provide this abstraction. It's defined as buckeScriptPackages.lib.buildBsPlatform

@turboMaCk turboMaCk force-pushed the turboMaCk:bs-add-7 branch from 822b4a4 to e1f49de Jan 11, 2020
@anmonteiro
Copy link
Member

@anmonteiro anmonteiro commented Jan 11, 2020

@turboMaCk thanks! That’s exactly what I suggested, so I’m happy indeed.

Something else I wonder now is if we need to maintain several versions of bs-platform inside nixpkgs. Given the current derivation, folks who want to downgrade / upgrade can probably just override the src, or call buildBsPlatform themselves.

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Jan 11, 2020

@anmonteiro this PR is actually not removing version 6, it just adds 7.0.1 and makes it default. the default version is accessible as bs-platform attribute, all versions (now 6.2.1 and 7.0.1) are available within buckleScriptPackages.*

@anmonteiro
Copy link
Member

@anmonteiro anmonteiro commented Jan 11, 2020

Yeah I’m suggesting we don’t maintain several versions.

Copy link
Contributor

@jonringer jonringer left a comment

please add

  badPlatforms = lib.platforms.aarch;

to prevent hydra trying to build this all the time :)

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Jan 12, 2020

@anmonteiro sorry I missunderstood your comment for some unexplainable reason, thanks for clarification.

@jonringer I will do that in case there is new PR though fixing aarch is main blocker for this PR and I'm not sure how to proceed with it myself.

@anmonteiro
Copy link
Member

@anmonteiro anmonteiro commented Jan 12, 2020

Tried to cross-compile this to aarch64 to investigate the build failure but I got nowhere and I probably don't really know what I'm doing.

@jonringer
Copy link
Contributor

@jonringer jonringer commented Jan 12, 2020

you could always comment:

  badPlatforms = lib.platforms.aarch; # should be able to work, however, lack hardware necessary to ensure successful build

The intent of marking as bad is just to have the evaluation reflect the build results.

@anmonteiro
Copy link
Member

@anmonteiro anmonteiro commented Jan 12, 2020

@jonringer I'm not sure what that would accomplish. Are you suggesting (as per your previous comment too) that we should just mark this derivation as not buildable in aarch64?

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Jan 13, 2020

@jonringer previous version (6.2.1) builds on arch and as far as we can tell this one should build as well so there is either some issue with build definition or in upstream.

@turboMaCk turboMaCk force-pushed the turboMaCk:bs-add-7 branch from e1f49de to 4508fad Jan 14, 2020
@turboMaCk turboMaCk force-pushed the turboMaCk:bs-add-7 branch from 4508fad to a898b28 Jan 14, 2020
@jonringer
Copy link
Contributor

@jonringer jonringer commented Jan 14, 2020

@GrahamcOfBorg build bs-platform

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Jan 14, 2020

Updates

  • As recommended by @bobzhang we're now using BS' fork of ninja
  • As it seems difficult to fix and debug aarch build failure it's now blacklisted
  • After some consideration I've decided to for now just simply update existing bs-platform attribute.

Details

ARM removal

I have no idea why the aarch64 build fails or how to debug it. I've opened issue. The only suggestion so far is that build runs out of memory.

No buckleScriptPackages.bs-platform-6

After giving it a bit more thought I think we should stick to simple variant of supporting of just a single version of bs-platform as part of nixpkgs. Users who need more control can use this project. I believe we need to understand the usecases more before we commit to maintaining more builds in nixpkgs.

Copy link
Member

@anmonteiro anmonteiro left a comment

@turboMaCk I like it. Thanks for taking care of this

@turboMaCk turboMaCk force-pushed the turboMaCk:bs-add-7 branch 2 times, most recently from 681e720 to 8cca242 Jan 14, 2020
@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Jan 14, 2020

I think I've finally managed to blacklist aarch the right way. In case someone will find it helpful nix-env -qa bs-platform --json -f . was helpful for debugging.

@ofborg ofborg bot requested a review from anmonteiro Jan 14, 2020
@jonringer
Copy link
Contributor

@jonringer jonringer commented Jan 14, 2020

sorry, the correct aarch platform is aarch64:

nix-repl> lib.platforms.aarch64
[ "aarch64-linux" "aarch64-darwin" "aarch64-none" ]
@jonringer
Copy link
Contributor

@jonringer jonringer commented Jan 14, 2020

but I think this is fine as aarch64-darwin is something like a tier 3 platform

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Jan 14, 2020

np... lets do it the right way

@turboMaCk turboMaCk force-pushed the turboMaCk:bs-add-7 branch from 8cca242 to db8f7d7 Jan 14, 2020
Copy link
Contributor

@jonringer jonringer left a comment

diff LGTM

[6 built, 9 copied (77.2 MiB), 12.5 MiB DL]
https://github.com/NixOS/nixpkgs/pull/76104
1 package were built:
bs-platform
pkgs/development/compilers/bs-platform/default.nix Outdated Show resolved Hide resolved
@turboMaCk turboMaCk force-pushed the turboMaCk:bs-add-7 branch from db8f7d7 to 43c7e81 Jan 14, 2020
@jonringer
Copy link
Contributor

@jonringer jonringer commented Jan 14, 2020

based on @anmonteiro 's approval. Merging

@jonringer jonringer merged commit 77752c6 into NixOS:master Jan 14, 2020
15 checks passed
15 checks passed
bs-platform on aarch64-linux No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
bs-platform on x86_64-linux Success
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
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

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