@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.

rebased

@GrahamcOfBorg build bs-platform

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.

@GrahamcOfBorg build buckeScriptPackages.bs-platform-6

@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.

@GrahamcOfBorg build buckeScriptPackages.bs-platform-6

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.

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
    "  ...  ";
}

@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

@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 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.

@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.*

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

@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.

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.

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.

@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?

@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.

@GrahamcOfBorg build bs-platform

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.

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.

sorry, the correct aarch platform is aarch64:

nix-repl> lib.platforms.aarch64
[ "aarch64-linux" "aarch64-darwin" "aarch64-none" ]

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

np... lets do it the right way

based on @anmonteiro 's approval. Merging