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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pkgs/development/compilers/bs-platform/bs-platform-62.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/bs-platform/bs-platform-62.nix Outdated Show resolved Hide resolved
@turboMaCk turboMaCk force-pushed the init-bs-platform branch 2 times, most recently from d4b3643 to 6f132bf Compare November 21, 2019 08:46
@turboMaCk turboMaCk changed the title bs-platform: init at 6.2.1 [wip] bs-platform: init at 6.2.1 Nov 23, 2019
@turboMaCk
Copy link
Member Author

Phases are refactored, binaries tested:

  • nixos
  • darwin

Comment on lines +34 to +32
rm -f vendor/ninja/snapshot/ninja.linux
cp ${ninja}/bin/ninja vendor/ninja/snapshot/ninja.linux
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@gamb
Copy link
Member

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

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 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 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 init-bs-platform branch 2 times, most recently from 9b710a1 to 3c5f790 Compare November 25, 2019 09:56
github = "gamb";
githubId = 293586;
name = "Adam Gamble";
};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gamb please check if this info is correct.

Copy link
Member

@gamb gamb Nov 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@turboMaCk Thanks, looks good 👍

@turboMaCk
Copy link
Member Author

@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
Copy link
Member Author

rebased onto master

@turboMaCk
Copy link
Member Author

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

@turboMaCk turboMaCk requested a review from FRidh December 5, 2019 11:41
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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

@jonringer
Copy link
Contributor

@GrahamcOfBorg build bs-platform

@turboMaCk
Copy link
Member Author

rebased again

@jonringer
Copy link
Contributor

@GrahamcOfBorg build bs-platform

@jonringer jonringer merged commit 0885502 into NixOS:master Dec 10, 2019
@turboMaCk
Copy link
Member Author

thanks so much @jonringer.

@ahdyt
Copy link

ahdyt commented Jul 2, 2020

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

@jonringer
Copy link
Contributor

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
Development

Successfully merging this pull request may close these issues.

None yet

7 participants