dbuenzli / topkg

The transitory OCaml software packager
http://erratique.ch/software/topkg
ISC License
69 stars 25 forks source link

Extend watermarking with edition commands #111

Closed ghost closed 7 years ago

ghost commented 7 years ago

This PR extends watermarking to add a few edition commands. The main motivation is to allow to put the %%VERSION%% string in a comment in the README.md file, so that it is not displayed on github. I don't like seeing it on the project page.

Implementation

Topkg.OS.File.write_edit

The first commit adds a function Topkg.OS.File.write_edit which extends write_subst:

(** Edition commands for {!write_edit}:
    {ul
    {- [`Replace_by s], replaces [%%ID%%] by [s].}
    {- [`Delete_bol], delete everything from the beginning of the
       current line to the end of [%%ID%%].}
    {- [`Delete_eol], deletes everything from the beginning [%%ID%%] to the
       end of the current line.}
    {- [`Delete_line], deletes the current line entirely.}}
*)
 type edition_command =
  [ `Replace_by of string
  | `Delete_bol
  | `Delete_eol
  | `Delete_line ]

val write_edit : fpath -> (string * edition_command) list -> string -> unit result
(** [write_edit file vars content] is like {!write} except any occurence
    of a string of the form ["%%ID%%"] in [content] is interpreted by the
    value of [List.assoc "ID" vars], if any. *)

I rewrote write_subst into two steps: a step that computes the substitutions and a step that applies them, to make it easier to detect overlapping substitutions.

Default bindings

The second commit adds the edition command to the type Topkg.Pkg.watermarks and to the default watermarks:

  val watermarks : watermark list
  (** [...]
     {- [("<<", `Delete_bol)]}
     {- [(">>", `Delete_eol)]}
     {- [("DELETE_LINE", `Delete_line)]}
     [...] *)

With this two commits, one can write this in its README.md file:

<!-- %%<<%%%%VERSION%%%%>>%% -->

Changing the default watermarks is a breaking change, so I can remove the default bindings if you prefer.

Explicit control of the version in opam files

The last commit adds an optional ?insert_version:bool argument to Topkg.Pkg.opam_file, allowing one to disable the automatic insertion of the version in opam files. This gives more control to the user. For instance, one can write:

...
#%%<<%%version: "%%VERSION%%"
version: "dev" #%%DELETE_LINE%%
...

This also makes the preparation step a fix point. Currently calling ocaml pkg/pkg.ml build --pinned true n times adds n version: ... lines to the opam files.

dbuenzli commented 7 years ago

Even though I find it essential I'm already not super happy with the watermarking process, so I'm really, really not enthusiastic to extend it with this kind of ugly text-based surgery (and add 150 lines to the project along the way). Especially if:

The main motivation is to allow to put the %%VERSION%% string in a comment in the README.md file, so that it is not displayed on github.

If that disturbs you too much I'd suggest to simply not add it. Or we could have an option to specify to add it at a well defined location on release by using the marked-up text parsing functions.

Regarding:

Explicit control of the version in opam files

I'm not against this but maybe we should rather review what opam does on vcs-pins. It seems to me that it picks the first of 1) the version specified on the cli 2) the version: field of the repo's opam file 3) the latest version available in the package repo and if there's no package with this name it uses ~unknown. One could argue that in step 3) it should simply use dev. This would avoid having to write version: "dev" in each of our repos and make the version string uniform in this case whether the package exists or not in the repo. /cc @AltGr

AltGr commented 7 years ago

It seems to me that it picks the first of 1) the version specified on the cli 2) the version: field of the repo's opam file 3) the latest version available in the package repo and if there's no package with this name it uses ~unknown.

As of opam 2, you should add 2a) the version: field in the included opam file, which can also be supplied using opam pin edit (or opam pin --edit). Also, ~dev is used rather than ~unknown.

dbuenzli commented 7 years ago

@AltGr but in opam2 if we are in 3) does it mean that pinning results in a greater version than anything in all cases and thus that we can avoid writing version: fields in the repo's opam files ?

AltGr commented 7 years ago

Hm, no, ~dev is actually before anything that doesn't start with ~... Any symbol other than ~ is guaranteed to be after letters & numbers though, so maybe +dev would be a better choice ?

dbuenzli commented 7 years ago

so maybe +dev would be a better choice ?

It seems to me that in general whenever you do not specify any version constrain (i.e. end up in 3.) you'd like the result to be unconstrained, i.e. greater than anything.

ghost commented 7 years ago

+dev seems a bit better than the current situation where opam reports one version number and ocamlfind reports another one:

$ opam pin add --dev react
[...]
$ opam info react -f version
1.2.1
$ ocamlfind list|grep react
react               (version: 1.2.0-18-gd9c6338)
react.top           (version: 1.2.0-18-gd9c6338)
$ grep Release .opam/4.04.0/doc/react/README.md 
Release v1.2.0-18-gd9c6338

Ideally all these version numbers should be the same.

Regarding watermarking in general, it seems to me that it could be mostly avoided by allowing something like odoc --let VERSION=...

dbuenzli commented 7 years ago

Regarding watermarking in general, it seems to me that it could be mostly avoided by allowing something like odoc --let VERSION=...

Not really, as I wrote here version numbers show up in many other places where you want them to be substituted (e.g. see topkg --version).

samoht commented 7 years ago

Sorry to hijack the discussion, but this is kind of related: for multi-packages repository, what is the recommend way to handle version constraints between the packages living in the same repo?

AltGr commented 7 years ago

and foo-bar.opam:

depends: [ "foo" {= "%%VERSION%%"}]

opam 2 allows depends: [ "foo" {= _:version} ] for this, meaning "at the same version".

My blog-post planned for thursday explains this :)

ghost commented 7 years ago

Independently of the default version when there is no version field, I still like to be able to choose the version topkg assigns to opam packages. For instance for Jane Street packages we must keep the v prefix.

For pinned packages, i think it'd be nice to allow using the output of git describe as topkg does, to get consistent version numbers. I suppose we could have a git-describe variable so that we could write this on the opam file:

version: git-describe

@AltGr, would something like this work for opam?

dbuenzli commented 7 years ago

I still like to be able to choose the version topkg assigns to opam packages.

I'm fine with this but not under this proposal. So here are the problems we need to solve:

a. The version number that gets chosen for dev packages by opam. b. The actual version number written by topkg distrib to the opam files that ship in the distribution.

The issue with b. is that this process needs to be performed on opam dev packages and thus by topkg (vs topkg-care) and that for bootstrapping reasons I do not want to use opam-format (which already pulls in a huge amount of deps) to update a version: field. Also I have absolutely no interest in adding version: git-describe in all the opam files of my repos, as I wrote elsewhere, "any non-project specific bit that has to be committed to these repositories is maintenance burden and a curse".

For these two reasons, topkg assumes you don't write version: fields in the opam files present in your repo --- which is self-evident for people who don't commit version numbers to their repos --- and it simply appends a version: field to them when it needs to. So I think we should try to keep this simple scheme:

  1. Do not write version: fields in the opam files of your repos.
  2. For topkg introduce an option to not strip the leading v in the version number written to the opam files on distribution preparation so that it satisifies Js versioning scheme.
  3. For opam make better decisions on which version number to use on dev packages.

Regarding 3. using +dev is there no version: in the opam file would already be a significant improvement but I wouldn't mind if opam did exactly what topkg does which is precisely defined here; the invocation ensures that in case there's no annotated tag a short hash is used or that if we try to describe HEAD a -dirty is appended to the version if the repo happens to be dirty. Corresponding (unlikely to be thoroughly tested) code for hg can be found here.

ghost commented 7 years ago

I do not want to use opam-format

There is also opam-file-format, which has no dependencies

any non-project specific bit that has to be committed to these repositories is maintenance burden and a curse

I totally agree with this. On this subject, I expect that most projects using jbuilder+topkg will have the same pkg/pkg.ml file, so it'd be great not to have to write it

  1. Do not write version: fields in the opam files of your repos.
    1. For topkg introduce an option to not strip the leading v in the version number written to the opam files on distribution preparation so that it satisifies Js versioning scheme.
    2. For opam make better decisions on which version number to use on dev packages.

Seems good to me. Opam using the same default version as topkg would indeed simplify everything.

BTW, why does topkg strips the leading v by default? Why not just use a tag without the leading v if you do not want it?

dbuenzli commented 7 years ago

There is also opam-file-format, which has no dependencies

But unfortunately it's barely usable for the smallest task (e.g. getting the deps out of of the dep: field) and it doesn't support layout preserving updates.

On this subject, I expect that most projects using jbuilder+topkg will have the same pkg/pkg.ml file, so it'd be great not to have to write it

We can certainly do something if you have a reasonable suggestion.

BTW, why does topkg strips the leading v by default? Why not just use a tag without the leading v if you do not want it?

On one hand the original semver specification mandated git tags had to have the leading v (also TBH at the time I didn't know you could have tags that started with numbers); this bit has been left out of v2.0.0 of the spec. On the other hand most people publish tarballs and version numbers on the opam repo without including a v, so it was an attempt at maximising compliance with existing conventions. Note that for watermarking both %%VERSION%% and %%VERSION_NUM%% are available.

ghost commented 7 years ago

We can certainly do something if you have a reasonable suggestion.

The best would be for topkg to use a standard pkg.ml file if there is no pkg/pkg.ml file and it detects it's running in a jbuilder project. And since this pkg.ml would be a constant it could just be a command. If the pkg.ml/command is not found topkg would suggest to install topkg-jbuilder.

The only difficulty would be detecting if a project is a jbuilder project. The only way to tell is to scan the source tree to see if it contains at least one jbuild file. Alternatively the manual suggest to have a jbuild-workspace.dev file at the root, so this could be used as a marker if scanning the project is not acceptable.

On one hand the original semver specification mandated git tags had to have the leading v (also TBH at the time I didn't know you could have tags that started with numbers); this bit has been left out of v2.0.0 of the spec. On the other hand most people publish tarballs and version numbers on the opam repo without including a v, so it was an attempt at maximising compliance with existing conventions. Note that for watermarking both %%VERSION%% and %%VERSION_NUM%% are available.

Ok

AltGr commented 7 years ago

There is also opam-file-format, which has no dependencies

But unfortunately it's barely usable for the smallest task (e.g. getting the deps out of of the dep: field) and it doesn't support layout preserving updates.

Layout preserving updates are supported since 2.0.0~beta3 (merged this morning). See also the new opam-ed that is just a CLI wrapper on top of it (but since that relies on Cmdliner, it's probably out of scope here anyway).

ghost commented 7 years ago

@AltGr what do you think of the idea of using the vcs in the same way as topkg does to get a default version?

@dbuenzli did what I describe looks like a reasonable suggestion to you? An alternative would be to provide enough hooks so that one can build an overlay topkg-jbuider command outside of topkg, but it'd be nicer if we could just use topkg directly.

dbuenzli commented 7 years ago

@dbuenzli did what I describe looks like a reasonable suggestion to you?

Yes.

dbuenzli commented 7 years ago

@AltGr what do you think of the idea of using the vcs in the same way as topkg does to get a default version?

I'd just file an opam issue for this, @AltGr always considers them and it will allow other non-topkg people to chime in about the adequacy of the scheme.

ghost commented 7 years ago

Alright, I'll open a PR about this on opam. I'm closing this PR then, I'll prepare two other PRs for:

  1. adding an option to keep the v prefix
  2. support jbuilder projects without a pkg/pkg.ml file