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: init at 6.2.1 #73570

Merged
merged 2 commits into from Dec 10, 2019
Merged

bs-platform: init at 6.2.1 #73570

merged 2 commits into from Dec 10, 2019

Conversation

@turboMaCk
Copy link
Member

@turboMaCk turboMaCk commented Nov 17, 2019

Motivation for this change

bs-platform is a BuckleScript compiler with set of additional tools. It is a OCaml/ReasonML compiler with javascript backend popular for building web and mobile apps. Official distribution is done via npm which doesn't work on NixOS (fails on build of native dependencies). This is an attempt to improve current status quo around bs-platform on NixOS full of workarounds.

There was some previous attempt using node2nix which was closed. This PR avoids usage of NPM alltogether.

In a meantime you can install bs-platform using this repository

Things done

bs-platform added to nixpkgs.

  • 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 @volth (who is maitainer listed as maintainer of ReasonML)

Credits

This is based on @hiroqn's project https://github.com/hiroqn/nix-bucklescript

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Nov 17, 2019

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/bs-platform-install/1520/6

@hiroqn
Copy link

@hiroqn hiroqn commented Nov 20, 2019

FYI:
I remove dependencies ocaml oPkgs.cppo oPkgs.camlp4 ninja by this commit

Copy link
Contributor

@jonringer jonringer left a comment

Apparently i forgot to submit this review from 3 days ago, sorry if old

@turboMaCk turboMaCk force-pushed the turboMaCk:init-bs-platform branch 2 times, most recently from d4b3643 to 6f132bf Nov 21, 2019
@turboMaCk turboMaCk changed the title bs-platform: init at 6.2.1 [wip] bs-platform: init at 6.2.1 Nov 23, 2019
@turboMaCk turboMaCk force-pushed the turboMaCk:init-bs-platform branch from a3f249d to a5ce929 Nov 23, 2019
@turboMaCk turboMaCk requested a review from jonringer Nov 23, 2019
@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Nov 23, 2019

Phases are refactored, binaries tested:

  • nixos
  • darwin
rm -f vendor/ninja/snapshot/ninja.linux
cp ${ninja}/bin/ninja vendor/ninja/snapshot/ninja.linux
Comment on lines +34 to +32

This comment has been minimized.

@turboMaCk

turboMaCk Nov 24, 2019
Author Member

this is the uglieast part as it seesm to be linux specific. That's being said build succeeds and binaries work even on macos

@turboMaCk turboMaCk changed the title [wip] bs-platform: init at 6.2.1 bs-platform: init at 6.2.1 Nov 24, 2019
@gamb
Copy link
Member

@gamb gamb commented Nov 24, 2019

The issue i'm seeing now is that calling the $(nix-build -A bs-platform)/bin/bsb from my bs project, I'll get:

File "bsconfig.json", line 1
Error: package bs-platform is not found 
It's the basic, required package. If you have it installed globally,
Please run `npm link bs-platform` to make it available

Basically because it'll look for bs-platform under node_modules/. It almost works to npm link under /nix/store/j1b50h2dpyds43dfm6gxpydjf6fn981h-bs-platform-6.2.1, but hits a snag trying to chmod -x inside a read-only directory.

Relevant discussion here: rescript-lang/rescript-compiler#3276

Maybe we want bsPackages in Nix too :)

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Nov 24, 2019

@gamb And how do you have your project setuped? I belive you need to have bs-platform as a dependecy in your node_modules as well and it has to be the same version. I just install project dependecies using yarn including the specified bs-platform and it works assuming you have this package installed on your system. Use nix-env -if . -A bs-platform and then install bs-platform 6.2.1 to your project using yarn/npm.

@turboMaCk turboMaCk force-pushed the turboMaCk:init-bs-platform branch from a5ce929 to eba95ee Nov 24, 2019
@gamb
Copy link
Member

@gamb gamb commented Nov 24, 2019

@turboMaCk Not sure that is working for me. My motivation is around not having to yarn add bs-platform (which is quite slow). Even if I install Nix bs-platform globally, then AFAICT yarn will go ahead and install a new copy of bsb under node_modules/:

> whereis bsb
bsb: /nix/store/qi30rksa95lscnh12m0sxaid8srj8rkj-user-environment/bin/bsb
> yarn
yarn install v1.17.3
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
Done in 134.67s.
> yarn run bsb
yarn run v1.17.3
$ /home/gamble/dev/reason-react/my-react-app/node_modules/.bin/bsb
bsb: no work to do.
Done in 0.10s.

The above is using the reason-react boilerplate. package.json contains:

"dependencies": {
  "react": "^16.8.1",
  "react-dom": "^16.8.1",
  "reason-react": ">=0.7.0"
},
"devDependencies": {
  "bs-platform": "^6.2.1",
  "moduleserve": "^0.8.4"
}

If I initialize my project with bsb then it'll symlink inside node_modules/, but that's not super useful for sharing environments:

> bsb -init my-react-app -theme react-hooks
Making directory my-react-app
Symlink bs-platform in /home/gamble/dev/reason-react/my-react-app 
> cd my-react-app/
> tree
.
├── bsconfig.json
├── node_modules
│   └── bs-platform -> /nix/store/rna07005x2vx9w66kckz40zrzx9zy1mh-bs-platform-6.2.1
├── package.json

Note that with the symlinked version above npm install takes ~1 second compared with ~130s when it has to install bs-platform from scratch. I found yarn overrides the symlink so doesn't work the same way.

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Nov 24, 2019

@gamb I think it is not possible to avoid yarn/npm install due to bsb internals now. I agree it is both slow and akward that it realies so much on nodejs package management but this is something that needs to be solved in upstream. For me this solves primarly the problem of not being able to install bs-platform on NixOS at all (yarn/npm installation failing on compilation of native depandencies).

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Nov 24, 2019

To explain more - yes yarn is expected to install bs platform to node_packages but it detects you already have binaries in system. Without this installed it would simply fail during yarn install. Usually people who really need this are either using some workarounds or building projects within docker.

@gamb
Copy link
Member

@gamb gamb commented Nov 24, 2019

@turboMaCk Thanks. Good to sanity check that it's a pain for everyone then 😄 the BS thread I linked to should hopefully yield something.

Since 6.x I'm finding that yarn is OK installing bs-platform on NixOS:

> nix-shell --pure -E 'with import <nixpkgs> {}; mkShell { name = "bs-env"; buildInputs = [ python nodejs yarn ]; }' --run "yarn add bs-platform@6.2.1"
yarn add v1.17.3
info No lockfile found.
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
success Saved lockfile.
success Saved 1 new dependency.
info Direct dependencies
└─ bs-platform@6.2.1
info All dependencies
└─ bs-platform@6.2.1
Done in 128.95s.

But it's slow. This Nix expression definitely gets me closer to a shareable Reason/BS environment anyway, so thanks :)

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Nov 24, 2019

@gamb That is interesting because for me it's failing.

......
if false; then \
  echo '#!/home/marek/.config/yarn/global/node_modules/bs-platform/native/4.02.3/bin/ocamlrun' > camlheader && \
  echo '#!/home/marek/.config/yarn/global/node_modules/bs-platform/native/4.02.3/bin/ocamlrun' > target_camlheader && \
  echo '#!/home/marek/.config/yarn/global/node_modules/bs-platform/native/4.02.3/bin/ocamlrund' > camlheaderd && \
  echo '#!/home/marek/.config/yarn/global/node_modules/bs-platform/native/4.02.3/bin/ocamlrund' > target_camlheaderd && \
  echo '#!' | tr -d '\012' > camlheader_ur; \
else \
  for suff in '' d; do \
    gcc -O -fno-defer-pop -Wall -D_FILE_OFFSET_BITS=64 -Wl,-E \
              -DRUNTIME_NAME='"/home/marek/.config/yarn/global/node_modules/bs-platform/native/4.02.3/bin/ocamlrun'$suff'"' \
              header.c -o tmpheader && \
    strip tmpheader && \
    mv tmpheader camlheader$suff && \
    gcc -O -fno-defer-pop -Wall -D_FILE_OFFSET_BITS=64 -Wl,-E \
              -DRUNTIME_NAME='"/home/marek/.config/yarn/global/node_modules/bs-platform/native/4.02.3/bin/ocamlrun'$suff'"' \
              header.c -o tmpheader && \
    strip tmpheader && \
    mv tmpheader target_camlheader$suff; \
  done && \
  cp camlheader camlheader_ur; \
fi
strip: 'tmpheader': No such file
strip: 'tmpheader': No such file
../boot/ocamlrun ../boot/ocamlc -strict-sequence -w +33..39 -g -warn-error A -bin-annot -nostdlib -safe-string `./Compflags camlinternalFormatBasics.cmo` -c camlinternalFormatBasics.ml
../boot/ocamlrun ../boot/ocamlc -strict-sequence -w +33..39 -g -warn-error A -bin-annot -nostdlib -safe-string `./Compflags pervasives.cmi` -c pervasives.mli
strip: 'tmpheader': No such file
mv: cannot stat 'tmpheader': No such file or directory
make[2]: *** [Makefile:50: camlheader_ur] Error 1
make[2]: *** Waiting for unfinished jobs....
mv: cannot stat 'tmpheader': No such file or directory
make[2]: *** [Makefile:50: target_camlheader] Error 1
make[2]: Leaving directory '/home/marek/.config/yarn/global/node_modules/bs-platform/ocaml/stdlib'
make[1]: *** [Makefile:194: coldstart] Error 2
make[1]: Leaving directory '/home/marek/.config/yarn/global/node_modules/bs-platform/ocaml'
make: *** [Makefile:144: world.opt] Error 2
child_process.js:669
    throw err;
    ^

Error: Command failed: make -j9 world.opt && make install 
    at checkExecSyncError (child_process.js:629:11)
    at Object.execSync (child_process.js:666:13)
    at Object.build (/home/marek/.config/yarn/global/node_modules/bs-platform/scripts/buildocaml.js:73:8)
    at provideCompiler (/home/marek/.config/yarn/global/node_modules/bs-platform/scripts/install.js:339:34)
    at Object.<anonymous> (/home/marek/.config/yarn/global/node_modules/bs-platform/scripts/install.js:365:20)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)

At least in case of yarn global add bs-platform.

When it comes to avoiding installation via yarn I think you can define shell.nix with shellHook in which you can do the symlink of the package to node_modules though I haven't tried that myself.

@turboMaCk turboMaCk force-pushed the turboMaCk:init-bs-platform branch from eba95ee to 458fbdd Nov 24, 2019
@turboMaCk turboMaCk changed the title bs-platform: init at 6.2.1 [WIP] bs-platform: init at 6.2.1 Nov 24, 2019
@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Nov 24, 2019

I'm marking rather as WIP until we resolve all questions around how this suppose to interact with node_modules.

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Nov 24, 2019

@gamb So I was looking more into this and I uncovered several things:

  • I forgot to add cp of package.json and bsconfig.json during refactoring - fixed
  • yarn always install bs-platform even if it's liked after bsb -init for some reason
  • npm on the other hand doesn't do that so using npm you can do what you need

This is an example shell.nix which avoid all the installations of bs-platform using npm/yarn.

{ pkgs ? import <nixpkgs> {} }:
with pkgs;
let
  bs-platform-src =
    fetchFromGitHub {
      owner = "turboMaCk";
      repo = "bs-platform.nix";
      rev = "c20e8dc8703ad7975c99d76b5779d31c86078d98";
      sha256 = "06wii6487crawi7ngbls59snvygqhh29jz5f9q106m3vp9jzy7h9";
    };
  bs-platform =
    import "${bs-platform-src}/bs-platform.nix"
      { inherit stdenv fetchFromGitHub ninja nodejs python35; };
in
mkShell {
    buildInputs = [ bs-platform nodejs ];

    shellHook = ''
      mkdir -p node_modules
      ln -sf ${bs-platform} node_modules/bs-platform
      echo "bs-platform linked to $(pwd)/node_modules/bs-platform"

      npm install
    '';
}

Does this work for you? Is it a good enough solution or do you have any ideas about improvements.

Also since you seems to be invested, would you mind if I added you as a 2nd maitainer of this package in the meta?

@turboMaCk turboMaCk changed the title [WIP] bs-platform: init at 6.2.1 bs-platform: init at 6.2.1 Nov 24, 2019
@gamb
Copy link
Member

@gamb gamb commented Nov 25, 2019

@turboMaCk Thanks! Yes interested in hacking on this some more, so feel free to add me to maintainers.

That shell works pretty nicely for my purposes - will experiment some more with it. Only issue I ran into is that the symlink won't apply properly if the node_modules/bs-platform link already exists so found I needed -n to handle that:

ln -sfn ${bs-platform} ./node_modules/bs-platform

For interop with npm scripts (i.e. npm run bsb) you'd need a second symlink like:

ln -sfn ${bs-platform}/bin/* ./node_modules/.bin
@turboMaCk turboMaCk force-pushed the turboMaCk:init-bs-platform branch 2 times, most recently from 9b710a1 to 3c5f790 Nov 25, 2019
github = "gamb";
githubId = 293586;
name = "Adam Gamble";
};

This comment has been minimized.

@turboMaCk

turboMaCk Nov 25, 2019
Author Member

@gamb please check if this info is correct.

This comment has been minimized.

@gamb

gamb Nov 25, 2019
Member

@turboMaCk Thanks, looks good 👍

@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Nov 26, 2019

@jonringer if you find a time to review this again feel free to do so. I think we've cleared everything within the code.

@turboMaCk turboMaCk force-pushed the turboMaCk:init-bs-platform branch from 3c5f790 to a2e38ba Dec 5, 2019
@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Dec 5, 2019

rebased onto master

@turboMaCk turboMaCk force-pushed the turboMaCk:init-bs-platform branch from d1162dc to f6033fd Dec 5, 2019
@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Dec 5, 2019

thanks for the review @FRidh. Build is now using python3 so this can be reviewed again.

@turboMaCk turboMaCk requested a review from FRidh Dec 5, 2019
Copy link
Contributor

@jonringer jonringer left a comment

would have preferred more commits (like maintainer addition being separate), but otherwise LGTM

executable seems to work

[4 built, 1 copied (45.6 MiB), 9.2 MiB DL]
https://github.com/NixOS/nixpkgs/pull/73570
1 package were built:
bs-platform
@turboMaCk turboMaCk force-pushed the turboMaCk:init-bs-platform branch from f6033fd to e8a5a89 Dec 5, 2019
@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Dec 5, 2019

@jonringer now splitted to two commits. Is it better?

@jonringer
Copy link
Contributor

@jonringer jonringer commented Dec 6, 2019

@GrahamcOfBorg build bs-platform

turboMaCk added 2 commits Dec 5, 2019
@turboMaCk turboMaCk force-pushed the turboMaCk:init-bs-platform branch from e8a5a89 to 542f4c5 Dec 8, 2019
@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Dec 8, 2019

rebased again

@jonringer
Copy link
Contributor

@jonringer jonringer commented Dec 10, 2019

@GrahamcOfBorg build bs-platform

@jonringer jonringer merged commit 0885502 into NixOS:master Dec 10, 2019
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
bs-platform on aarch64-linux Success
Details
bs-platform on x86_64-darwin Success
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
@turboMaCk
Copy link
Member Author

@turboMaCk turboMaCk commented Dec 10, 2019

thanks so much @jonringer.

@fnlaai
Copy link

@fnlaai fnlaai commented Jul 2, 2020

I'm sorry, which approach is able to do?

@jonringer
Copy link
Contributor

@jonringer jonringer commented Jul 2, 2020

I'm sorry, which approach is able to do?

?

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

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