Setup NPM Packaging #214

leostera · 2018-10-10T08:28:52Z

Opening this PR for visibility/referencing, but I'll be modifying the history heavily until I get something I'm comfortable with.

Setting up esy seemed straightforward, although adding dependencies one by one manually is always annoying.

This is missing a few things:

Pin tyxml/markup/etc to their appropriate versions (some tests are failing due to sexp indentation)
Run on Travis for switch 4.02.3
Trim down package (@jordwalke's suggestion)
~~Publish from Travis for switch 4.02.3 on master branch tag~~

But so far I've managed to install the npm package locally and it runs alright 👍

ostera/odoc λ make npm
esy install
info install 0.2.11
esy make build
dune build
esy release
info release 0.2.11
dune build
info Creating npm release
# ...so much output here exporting packages
info Exporting ocaml-4.2.3005-a85217cf: done
info Configuring release
info Done!

ostera/odoc λ cd _release && npm --global install .
/usr/local/bin/odoc -> /usr/local/lib/node_modules/odoc/bin/odoc

> odoc@1.2.0 postinstall /usr/local/lib/node_modules/odoc
> node ./esyInstallRelease.js

importing: opam__slash__uutf-1.0.1-1e184456
# another good deal of imports here
importing: esy_ocaml__slash__substs-0.0.1-441fe663
Done!
+ odoc@1.2.0
updated 1 package in 4.38s

odoc/_release λ /usr/local/lib/node_modules/odoc/bin/odoc
Available subcommands: compile, html, html-fragment, support-files, css, compile-deps,
html-deps, compile-targets, html-targets
See --help for more information.

odoc/_release λ /usr/local/lib/node_modules/odoc/bin/odoc --version
%%VERSION%%

If you can please give it a go!

cc/ @aantron @rizo @lpw25 @ryyppy @grabbou @avsm @ulrikstrid

ref #195

jordwalke · 2018-10-10T10:31:53Z

HI, I'm curious if this is the kind of package that actually benefits from being a globally installable command line utility from npm? (I am not yet familiar with odoc but eager to learn).

I did notice that you did not trim out any of the binary artifacts, so here's an example of how to do that. It makes the prebuilt release much leaner: https://github.com/esy-ocaml/hello-reason/blob/master/package.json#L13

dbuenzli · 2018-10-10T10:54:11Z

HI, I'm curious if this is the kind of package that actually benefits from being a globally installable command line utility from npm? (I am not yet familiar with odoc but eager to learn).

I'd say this is unlikely odoc which relies on cmt/cmti/cmi files is tied to the version of the compiler you used to generate these artefacts.

leostera · 2018-10-10T10:56:56Z

Hej @jordwalke as it is right now using it at all requires opam, which is a hindrance if you're using an bsb-only setup.

I'll grant you that the current usage flow is far from ideal (as you can see from the suggested script in the readme), but having a release pipeline for npm makes it easier to iterate and gather feedback from the other side of the community.

@dbuenzli maybe we should make it clear that this npm package is intended for usage with the 4.02.3+buckle-master switch? I'm open to ideas to enforce this too.

And 👍 will trim down the package when I get home 📦

aantron · 2018-10-10T14:00:03Z

@Ostera I don't think this package requires a switch that users need to be aware of. IIRC we can just choose the proper compiler version in esy.json, so that the binary is built on 4.02.3, and then it can be used with BuckleScript.

I don't think we need an opam switch around at runtime.

leostera · 2018-10-10T14:01:54Z

@aantron not at runtime, but they won't be able to generate docs from .cmt[i]s built with a different switch.

rizo · 2018-10-10T14:06:54Z

Once again working on BuckleScript and dune projects on the same machine is going to be hard with global installations...

aantron · 2018-10-10T14:17:41Z

@Ostera It's not the switch but the compiler version that matters (actually, the format of .cmt[i] files). As long as the OCaml version our build internally uses to build the odoc binary is the same as what's being used by BuckleScript (4.02.3), the user doesn't need to know about it. They only need an easy way of finding the binary, probably through NPM.

leostera · 2018-10-10T14:26:15Z

@rizo: Once again working on BuckleScript and dune projects on the same machine is going to be
hard with global installations...

You can always do local installs and ./node_modules/.bin/odoc

@aantron: It's not the switch but the compiler version that matters

My bad, still mix up the wording about it! This should work just as fine for any BuckleScript version.

jordwalke · 2018-10-10T21:25:16Z

@Ostera I don't think this package requires a switch that users need to be aware of. IIRC we can just choose the proper compiler version in esy.json, so that the binary is built on 4.02.3, and then it can be used with BuckleScript.

Yes, you can choose 4.02.3 in the esy.json file and things will work on any project that uses a 4.02.3 compiler including BuckleScript. I can just imagine that people are going to run into some issues when the try to run odoc on a local opam or esy project that uses a different version of the compiler. Still, it would be kind of cool to have a globally installed way to build docs. A couple of ideas/suggestions:

Make sure the error message is really friendly when the compiler version isn't what is expected.
Perhaps package up prebuilt binaries into a package names to avoid conflicts.

For the second point, what about making the prebuilt binaries under a package name of odoc-402. Ideally odoc would also build/install a binary named odoc402, and then you would list that in the releasedBinaries field. Then if all of that were true you could have multiple global npm prebuilt binaries existing globally, none of their package names conflicting, and none of the binaries they put in the global path conflicting. If it's too hard to change odoc to also produce a copy of the main binary under the name odoc402, then maybe at least make the hard dependency on compiler version apparent in the released binary package name.
I just wanted to avoid people getting a really bad error when they try to run odoc on a local Dune project on 4.06.
Thoughts?

jordwalke · 2018-10-10T21:27:28Z

.ci/install-esy.sh

+#!/bin/bash
+
+if [[ $ESY_BUILD == YES ]]; then
+  npm --global install esy


@andreypopp should this be pinned to some stable version?

yeah, esy@0.3.x would be relatively safe (considering this project requires features new in 0.3.x, otherwise it's better to pin to 0.2.x).

jordwalke · 2018-10-10T21:27:47Z

.travis.yml


 env:
  - OCAML=4.02.3
  - OCAML=4.02.3+buckle-master
+  - OCAML=4.02.3+buckle-master ESY_BUILD=YES ESY__CACHE=/home/travis/.esy


Can you help me understand what this accomplishes? (Using buckle-master?) I've never understood why people ever use buckle-master for things. I've never come across a scenario when it was needed.

We need to build odoc using this version of the compiler to be able to read .cmti files compiled with it.

But why do you need +buckle-master? Why can't you just use 4.02.3. Is the cmt data structure different for buck-master than for plain 4.02.3? I've used plain 4.02.3 to parse bs's .cmt files in the past without any trouble.

@Ostera AFAIK esy will use its own internal oap switch to build the odoc binary.

@aantron @jordwalke I left that in there because I was installing opam/ocaml even for the esy build. I realize now that it's not needed at all, so I will remove it and make the installation optional.

andreypopp · 2018-10-10T21:56:31Z

package.json

+    "Leo White <leo@lpw25.net>"
+  ],
+  "esy": {
+    "test": "make test",


note that this does nothing, you need to put it into "script.test" if you want to run esy test.

Wasn't using it anyway, will remove!

leostera · 2018-10-10T22:42:22Z

Alright! This looks a little tidier now, let's do another round of reviews ⚔️ — I took the liberty to optimize the CI process, not to have to wait ~7 mins on every change, but instead ~2. Feedback welcome there too (I'm open to submitting a separate PR for those changes if need be).

@aantron: Just like with the automatic publishing of docs, let's keep this publishing manual. Any advice on caching the opam/ocaml installations any futher?

@jordwalke, @andreypopp: Could you provide insights on how to properly cache the build directory of esy? I'm noticing that dune runs again, rebuilding and retesting the whole project rather than doing it incrementally.

@rizo: is there anything you'd like changed about this PR that would make you more comfortable with the way we would publish the odoc binary? I've been thinking about this and I think the approach I'd like best would be an odoc release that understands several compiler versions (so you only need one for every compiler), but I might be missing some underlying complexity of developing/building/distributing such a binary.

@ryyppy @grabbou @dbuenzli: would like to hear your thoughts on this too 🗣

leostera · 2018-10-10T22:45:25Z

@jordwalke: I just wanted to avoid people getting a really bad error when they try to run odoc on
a local Dune project on 4.06.

Shouldn't they be running it with esy dune build @doc? which should use the opam installed version that's appropriate for their compiler version, if I understood that particular esy usage.

rizo · 2018-10-10T23:53:43Z

.ci/install-esy.sh

+  npm --global install esy@0.3.x
+  esy --version
+else
+  echo "Nothing to be done."


Is this else branch needed? Seeing this message in the log is not informative IMO.

It looked strange to see the script marked as ran but no output whatsoever. Perhaps I can change the text to "Skipping esy installation." ?

Ended up joining the install scripts after removing the OCAML env variable from the esy build. This is no longer an issue.

rizo · 2018-10-10T23:55:46Z

.ci/install.sh

@@ -0,0 +1,11 @@
+#!/bin/bash


This might be some convention but I feel like this script should be called setup, init or install-deps.

I like install-deps. Conveys a little more info about what's being installed.

rizo · 2018-10-10T23:57:08Z

.travis.yml

+  - /home/travis/.opam
+  - /home/travis/.esy
+  - /home/travis/.nvm
+  - ./_build/default


Is this desirable? Do we want to keep cache even for tests executions across builds?

I had the same question. You only want to cache builds of your dependencies typically (the ~/.esy/ directory).

Coming from bazel I'm used to letting the build tool cache very aggressively — assuming we trust dune to re-build and re-test incrementally, should this be a problem?

I would be fine with this if we cleaned the html _scratch directory between test executions. Otherwise the previously generated files that are not generated in next runs might interfere with the result (i.e., it should be an error in this case).

Good callout! Seems like a good before_script. Let's see how it works.

rizo · 2018-10-10T23:58:25Z

Makefile

@@ -2,6 +2,19 @@
 build :
 	dune build

+.PHONY : npm


make npm seems a bit ambigious to me. Again maybe this is some sort of convention but I’d call it npm-publish.

I don't think this should be called npm-publish, because, as I understand, it just packages things in a local _release directory. But it shouldn't be called npm either.

make npm-release ?

-release has the danger of being interpreted as the verb, which makes it seem dangerous. How about npm-package?

rizo · 2018-10-11T00:00:07Z

Makefile

+	esy build
+
+.PHONY : npm-test
+npm-test :


This seems very meta (ie a make command that calls esy that calls make again). How is it different to make test?

@rizo This runs make test in the esy environment. It doesn't seem too bad.

Alternatively, we can just call esy make test, but I fear that from looking at the files we currently have it won't be obvious that this should be run (at some point) to make sure the build works as an npm package.

We can also document it in the contribution guidelines.

rizo · 2018-10-11T00:02:21Z

package.json

+  "name": "odoc",
+  "version": "1.2.0",
+  "description": "The OCaml/Reason Documentation Generator",
+  "main": "index.js",


Hm, are you trying to sneak in some uncommited JS? 😛

I believe that will be a generated file.

EDIT: well, we are not targeting JS at all, so I'm not sure why we need this.

This was generated. Should be removed from the package.json!

rizo · 2018-10-11T00:10:51Z

I might be missing some underlying complexity of developing/building/distributing such a binary

The ability to work with cmt/cmti/cmi files produced by different compiler versions is currently hard because of inconsistencies across typed parsetrees. We would need a typed version of ocaml-migrate-parsetree which doesn’t exist AFAIK. The ad-hoc approach would be too painfull.

rizo · 2018-10-11T00:26:38Z

@jordwalke regarding your suggestion to add a version suffix to the binary name: it would avoid the conflict with the opam-installed version but I don’t understand how the build tools would find the needed versioned binaries..

aantron · 2018-10-11T00:27:47Z

The ability to work with cmt/cmti/cmi files produced by different compiler versions is currently hard because of inconsistencies across typed parsetrees. We would need a typed version of ocaml-migrate-parsetree which doesn’t exist AFAIK. The ad-hoc approach would be too painfull.

@rizo This might not actually be a problem for odoc, in principle.

odoc already has enough code to load Typedtrees from compiler versions 4.02-4.07, and largely convert them into its own representation. (src/model).

This leaves the remaining direct uses of Typedtree. Typedtree is used only in the loaders themselves cmt.ml, cmti.ml, and ident_env.ml. That's where (almost?) all of our conditional compilation is, too, but it doesn't look too bad.

It may be possible to link loaders for all OCaml versions into a single odoc binary, and reconcile them somehow. The big issue would be how to avoid duplicating the vast majority of the shared code. Perhaps we can do it with a functor or some preprocessing. We would also likely need to vendor a copy of the relevant files in compiler-libs for each version of the compiler.

rizo · 2018-10-11T00:33:51Z

@aantron Thanks for clarifying that. I’m not sure I like the megabinary idea :) But having cross-version portability would be very nice indeed.

aantron · 2018-10-11T00:34:50Z

.ci/build.sh

@@ -0,0 +1,11 @@
+#!/bin/bash


I suggest to rename directory .ci/ to test/ci/, so it doesn't have to be hidden, and doesn't clutter the GitHub repo.

aantron · 2018-10-11T00:36:50Z

.travis.yml


 env:
  - OCAML=4.02.3
  - OCAML=4.02.3+buckle-master
+  - OCAML=4.02.3+buckle-master ESY_BUILD=YES ESY__CACHE=/home/travis/.esy


@Ostera AFAIK esy will use its own internal oap switch to build the odoc binary.

aantron · 2018-10-11T00:39:35Z

.travis.yml

+  - ./_build/default
+
+install:
+  - ./.ci/install.sh


I recommend not breaking this up into files. If you'd like to move it to a shell script (for conditionals?), I suggest to create one shell script, and do things there. Perhaps one for building and one for installing, if caching happens between those two steps.

With many scripts, and some commands in the .travis.yml file, people have to look through a lot of little files to understand what is going on.

@aantron a small problem with this approach is that the eval $(opam env) call has to be made from an individual line in the .travis.yml file to affect the rest of the build.

eval from within scripts only affects the current environment, not it's parent.

We can bundle the install.sh and install-esy.sh, but we'd need a separate script that is run after the eval to be able to output the ocaml/opam -[-]version commands.

install: - ./.ci/install.sh - "if [[ $OCAML ]]; then eval `opam config env`; fi" - ./.ci/print-versions.sh

Let me know what you think.

aantron · 2018-10-11T00:42:43Z

Makefile

@@ -2,6 +2,19 @@
 build :
 	dune build

+.PHONY : npm


I don't think this should be called npm-publish, because, as I understand, it just packages things in a local _release directory. But it shouldn't be called npm either.

aantron · 2018-10-11T00:44:17Z

Makefile

+	esy build
+
+.PHONY : npm-test
+npm-test :


@rizo This runs make test in the esy environment. It doesn't seem too bad.

aantron · 2018-10-11T00:44:39Z

package.json

+  "name": "odoc",
+  "version": "1.2.0",
+  "description": "The OCaml/Reason Documentation Generator",
+  "main": "index.js",


I believe that will be a generated file.

EDIT: well, we are not targeting JS at all, so I'm not sure why we need this.

jordwalke · 2018-10-11T03:44:00Z

@jordwalke: I just wanted to avoid people getting a really bad error when they try to run odoc on
a local Dune project on 4.06.

Shouldn't they be running it with esy dune build @doc? which should use the opam installed version that's appropriate for their compiler version, if I understood that particular esy usage.

Perhaps but when BS upgrades to 4.06 the same thing will happen and we'll need to clearly make sure the user knows what version their globally installed odoc is compatible with. At least a good error message is in order. I don't have too much to add besides that. Happy to see odoc get into more hands either way.

leostera · 2018-10-11T19:48:23Z

@aantron @rizo I've addressed most of your suggestions. Let me know how you want to proceed!

I understand there's an open question of how would this odoc installation work with opam switching compilers, but from a practical point of view:

dune/esy users will run it through dune build @doc,
npm users can install it locally and run it via runscripts or ./node_modules/.bin/odoc

So I don't see it as a blocker at the moment. Happy to be shown wrong! 🙌

aantron · 2018-10-11T21:08:29Z

@Ostera I don't think there's any issue. The esy/NPM odoc does not interfere with opam switches. It's how you described in your list.

aantron

It looks pretty much fine to me. We can work out any remaining quirks around release time, and maybe refactor the CI scripts over time.

aantron · 2018-10-11T21:14:05Z

.travis.yml

-language: generic
+language: node_js
+node_js:
+  - "10"


What is using this?

Installing esy requires a node environment, which we can either manage ourselves or let Travis manage for us.

+ Create package.json for 4.02.3005 switch with necessary dependencies + Make targets to build, test, and package for npm + Ignore esy output folders

This will cache the opam switch and parts of the dune build, making sure that the test-generated files (located in the `_scratch` folder) are always cleaned up before running the tests.

aantron · 2018-10-11T22:27:48Z

Thanks @Ostera and the reviewers!

leostera force-pushed the npm-packaging branch from 28092f1 to e7a05fb Compare October 10, 2018 08:36

leostera force-pushed the npm-packaging branch 2 times, most recently from 05d6dc2 to 2516057 Compare October 10, 2018 20:47

jordwalke reviewed Oct 10, 2018

View reviewed changes

andreypopp reviewed Oct 10, 2018

View reviewed changes

leostera force-pushed the npm-packaging branch 2 times, most recently from 59fc37c to c83921e Compare October 10, 2018 22:37

rizo reviewed Oct 10, 2018

View reviewed changes

rizo reviewed Oct 11, 2018

View reviewed changes

aantron reviewed Oct 11, 2018

View reviewed changes

leostera force-pushed the npm-packaging branch 2 times, most recently from 54e4251 to 14aa5a5 Compare October 11, 2018 06:56

leostera mentioned this pull request Oct 11, 2018

Investigate supporting multiple compilers in a single odoc binary #215

Closed

leostera force-pushed the npm-packaging branch from fb60d2c to 19524c5 Compare October 11, 2018 20:05

aantron reviewed Oct 11, 2018

View reviewed changes

leostera added 3 commits October 11, 2018 23:58

Setup npm packaging

a4a487c

+ Create package.json for 4.02.3005 switch with necessary dependencies + Make targets to build, test, and package for npm + Ignore esy output folders

Build, test, and package for npm on Travis

a1f3d2c

Cache opam switch and dune build modulo _scratch

049b4bf

This will cache the opam switch and parts of the dune build, making sure that the test-generated files (located in the `_scratch` folder) are always cleaned up before running the tests.

leostera force-pushed the npm-packaging branch from 19524c5 to 049b4bf Compare October 11, 2018 22:03

aantron merged commit 7a8a8a0 into ocaml:master Oct 11, 2018

This was referenced Oct 11, 2018

Publish as an npm installable package #195

Closed

odoc 1.3.0: OCaml documentation generator ocaml/opam-repository#12811

Merged

Setup NPM Packaging #214

Setup NPM Packaging #214

Conversation

leostera commented Oct 10, 2018 • edited Loading

jordwalke commented Oct 10, 2018

dbuenzli commented Oct 10, 2018

leostera commented Oct 10, 2018 • edited Loading

aantron commented Oct 10, 2018

leostera commented Oct 10, 2018

rizo commented Oct 10, 2018

aantron commented Oct 10, 2018

leostera commented Oct 10, 2018

jordwalke commented Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordwalke Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leostera commented Oct 10, 2018

leostera commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordwalke Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rizo Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aantron Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rizo commented Oct 11, 2018

rizo commented Oct 11, 2018

aantron commented Oct 11, 2018

rizo commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leostera Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aantron Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

jordwalke commented Oct 11, 2018

leostera commented Oct 11, 2018

aantron commented Oct 11, 2018

aantron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aantron commented Oct 11, 2018

leostera commented Oct 10, 2018 •

edited

Loading

leostera commented Oct 10, 2018 •

edited

Loading

jordwalke commented Oct 10, 2018 •

edited

Loading

jordwalke Oct 10, 2018 •

edited

Loading

jordwalke Oct 11, 2018 •

edited

Loading

rizo Oct 11, 2018 •

edited

Loading

aantron Oct 11, 2018 •

edited

Loading

leostera Oct 11, 2018 •

edited

Loading

aantron Oct 11, 2018 •

edited

Loading