dune subst reads from dune-project #2955

fangyi-zhou · 2019-12-05T22:10:41Z

Signed-off-by: Fangyi Zhou me@fangyi.io

Working in progress for #2910

fangyi-zhou · 2019-12-05T22:11:16Z

Well, I meant to create a draft pull-request but misclicked

src/dune/watermarks.ml

ghost · 2019-12-10T08:55:44Z

src/dune/watermarks.ml

@@ -173,7 +173,7 @@ let subst_file path ~map =

 (* Minimal API for dune-project files that makes as little assumption about the
   contents as possible and keeps enough info for editing the file. *)
-module Dune_project = struct
+module Dune_project_mini = struct


Initially, the intent of this sub-module was to not do a complete decoding of the dune-project file and keep enough information to edit it. However, now that we are fully loading the dune-project file it makes less sense to also load it again in a minimal way on the side. At this point we are only relying on this module for editing the dune-project file, so I propose to rename it Dune_project_editor instead, to state its purpose more clearly.

We could also extend the main Dune_project module to keep the necessary information to do the edition. It's not a requirement for this PR, but you are welcome to have a look if you feel motivated :)

src/dune/watermarks.ml

ghost · 2019-12-10T09:00:35Z

No worries for the draft status. I left a couple of comments but otherwise it looks good to me!

Could you also add a test for this please? To do so, you need to add a directory with a run.t file in test/blackbox-tests/test-cases. Then run dune runtest --auto-promote once to update the boilerplate and then dune runtest or dune build @name-of-your-test to run the test. The format of tests is cram, i.e. lines starting with $ are commands to run and lines starting with two spaces are the expected output of the commands. You can use dune runtest followed by dune promote to automatically update the run.t file with the actual output of commands.

fangyi-zhou · 2019-12-10T21:45:37Z

Still working in progress.

I don't see any way to modify the dune project by the exported API, so I guess we'll have to leave the editor here for the moment being. I changed the editor module to extend the original Dune_project module.

With regards to testing, I see we're having a problem. Now dune-project files takes precedence over .opam files, so some substitution tests are failing due to non-backward compatible behaviour. Should it be the case, or we should first check for .opam file, then look for dune-project file to preserve some backward compatibility?

ghost · 2019-12-11T12:20:38Z

I added [WIP] in the title to make it clear that it is still in progress, you can remove it when you consider it ready.

I don't see any way to modify the dune project by the exported API, so I guess we'll have to leave the editor here for the moment being. I changed the editor module to extend the original Dune_project module.

Seems good to me. The current code looks clear.

With regards to testing, I see we're having a problem. Now dune-project files takes precedence over .opam files, so some substitution tests are failing due to non-backward compatible behaviour. Should it be the case, or we should first check for .opam file, then look for dune-project file to preserve some backward compatibility?

Hmm, that's odd. I'm looking at the failing tests and I would have expected the information to be capture by the Package.t value in the Dune_project.t one, and so for the variable to still be substituted.

ghost · 2019-12-11T14:05:11Z

Ah ok, I just remembered that we don't parse all the fields when reading opam files in Dune_project. The code is at the end of dune_project.ml, in the load function. After reading the opam file, we generate a Package.t record where most fields are set to dummy values. We only read the version.

One possibility is to change this code to instead read the data from the opam file. We can use the code in make_watermark_from_opam_file for that. We can then extract it into a function that turns an opam file into a Package.t and use this function in make_watermarl_from_opam_file, so that we use the same code path whether we have a dune-project file or not.

ghost · 2020-01-15T15:57:32Z

@fangyi-zhou did you have a chance to take another look at this issue since last time?

fangyi-zhou · 2020-01-15T18:10:00Z

I'll try to work on it a bit tonight, otherwise I'm fine if someone would like to take over from me, or start a new PR

ghost · 2020-01-16T09:55:02Z

Ok, let me know how it goes. Happy to help finishing the PR.

Signed-off-by: Fangyi Zhou <me@fangyi.io>

We only need to know whether the package has a corresponding opam file or not. Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

ghost · 2020-01-16T16:09:28Z

Superposing the two sources of information is not ideal. Dune makes sure there is only one source of information written by the user: the opam files or the dune-project file. But we can't have both. I'm refactoring a bit the code to make that more explicit.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

Instead of merging them later on Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

ghost · 2020-01-16T16:47:29Z

I reworked a bit the code. Now all the information is collected and merged right from the start and contained in Dune_info.packages ... and there is only one way to create a watermark map in watermarks.ml.

Merging this now. Thanks for your contribution to Dune!

fangyi-zhou · 2020-01-16T16:48:15Z

Thanks a lot for the help!

@shonfeder

…lugin, dune-private-libs and dune-glob (2.2.0) CHANGES: - `dune test` is now a command alias for `dune runtest`. This is to make the CLI less idiosyncratic (ocaml/dune#3006, @shonfeder) - Allow to set menhir flags in the `env` stanza using the `menhir_flags` field. (ocaml/dune#2960, fix ocaml/dune#2924, @bschommer) - By default, do not show the full command line of commands executed by `dune` when `dune` is executed inside `dune`. This is to make integration tests more reproducible (ocaml/dune#3042, @diml) - `dune subst` now works even without opam files (ocaml/dune#2955, fixes ocaml/dune#2910, @fangyi-zhou and @diml) - Hint when trying to execute an executable defined in the current directory without using the `./` prefix (ocaml/dune#3041, fixes ocaml/dune#1094, @voodoos). - Extend the list of modifiers that can be nested under `with-accepted-exit-codes` with `chdir`, `setenv`, `ignore-<outputs>`, `with-stdin-from` and `with-<outputs>-to` (ocaml/dune#3027, fixes ocaml/dune#3014, @voodoos) - It is now an error to have a preprocessing dependency on a ppx rewriter library that is not marked as `(kind ppx_rewriter)` (ocaml/dune#3039, @snowleopard). - Fix permissions of files promoted to the source tree when using the shared cache. In particular, make them writable by the user (ocaml/dune#3043, fixes ocaml/dune#3026, @diml) - Only detect internal OCaml tools with `.opt` extensions. Previously, this detection applied to other binaries as well (@kit-ty-kate, @rgrinberg, ocaml/dune#3051). - Give the user a proper error message when they try to promote into a source directory that doesn't exist. (ocaml/dune#3073, fix ocaml/dune#3069, @rgrinberg) - Correctly build vendored packages in `-p` mode. These packages were incorrectly filtered out before. (ocaml/dune#3705, @diml) - Do not install vendored packages (ocaml/dune#3704, @diml) - `make` now prints a message explaining the main targets available (ocaml/dune#3085, fix ocaml/dune#3078, @diml) - Add a `byte_complete` executable mode to build programs as self-contained bytecode programs (ocaml/dune#3076, fixes ocaml/dune#1519, @diml)

@shonfeder

…lugin, dune-private-libs and dune-glob (2.2.0) CHANGES: - `dune test` is now a command alias for `dune runtest`. This is to make the CLI less idiosyncratic (ocaml/dune#3006, @shonfeder) - Allow to set menhir flags in the `env` stanza using the `menhir_flags` field. (ocaml/dune#2960, fix ocaml/dune#2924, @bschommer) - By default, do not show the full command line of commands executed by `dune` when `dune` is executed inside `dune`. This is to make integration tests more reproducible (ocaml/dune#3042, @diml) - `dune subst` now works even without opam files (ocaml/dune#2955, fixes ocaml/dune#2910, @fangyi-zhou and @diml) - Hint when trying to execute an executable defined in the current directory without using the `./` prefix (ocaml/dune#3041, fixes ocaml/dune#1094, @voodoos). - Extend the list of modifiers that can be nested under `with-accepted-exit-codes` with `chdir`, `setenv`, `ignore-<outputs>`, `with-stdin-from` and `with-<outputs>-to` (ocaml/dune#3027, fixes ocaml/dune#3014, @voodoos) - It is now an error to have a preprocessing dependency on a ppx rewriter library that is not marked as `(kind ppx_rewriter)` (ocaml/dune#3039, @snowleopard). - Fix permissions of files promoted to the source tree when using the shared cache. In particular, make them writable by the user (ocaml/dune#3043, fixes ocaml/dune#3026, @diml) - Only detect internal OCaml tools with `.opt` extensions. Previously, this detection applied to other binaries as well (@kit-ty-kate, @rgrinberg, ocaml/dune#3051). - Give the user a proper error message when they try to promote into a source directory that doesn't exist. (ocaml/dune#3073, fix ocaml/dune#3069, @rgrinberg) - Correctly build vendored packages in `-p` mode. These packages were incorrectly filtered out before. (ocaml/dune#3705, @diml) - Do not install vendored packages (ocaml/dune#3704, @diml) - `make` now prints a message explaining the main targets available (ocaml/dune#3085, fix ocaml/dune#3078, @diml) - Add a `byte_complete` executable mode to build programs as self-contained bytecode programs (ocaml/dune#3076, fixes ocaml/dune#1519, @diml)

fangyi-zhou mentioned this pull request Dec 5, 2019

dune subst fails on missing opam file #2910

Closed

ghost suggested changes Dec 10, 2019

View reviewed changes

ghost changed the title ~~dune subst reads from dune-project~~ [WIP] dune subst reads from dune-project Dec 11, 2019

This comment has been minimized.

Sign in to view

fangyi-zhou changed the title ~~[WIP] dune subst reads from dune-project~~ dune subst reads from dune-project Jan 16, 2020

fangyi-zhou added 3 commits January 16, 2020 12:28

dune subst reads from dune-project

8158fd0

Signed-off-by: Fangyi Zhou <me@fangyi.io>

Address some reviewer's comments

1bfa57f

Signed-off-by: Fangyi Zhou <me@fangyi.io>

Merge opam package and dune-project package info

2bab6c4

Signed-off-by: Fangyi Zhou <me@fangyi.io>

fangyi-zhou requested a review from a user January 16, 2020 12:29

Simplify Package.Kind

8517d39

We only need to know whether the package has a corresponding opam file or not. Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

jeremiedimino added 5 commits January 16, 2020 16:09

Parse opam files completely in Dune_project.load

4f307de

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

Collect all package informations right in Dune_project.t values

20bcb94

Instead of merging them later on Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

Remove unused subst test

cdb43d9

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

Update changes

908a890

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

Merge branch 'master' into dune-subst-reads-from-dune-project

aead24e

ghost merged commit 17c5bad into ocaml:master Jan 16, 2020

fangyi-zhou deleted the dune-subst-reads-from-dune-project branch January 16, 2020 16:48

rgrinberg mentioned this pull request Feb 6, 2020

[new release] dune-build-info, dune, dune-configurator, dune-action-plugin, dune-private-libs and dune-glob (2.2.0) ocaml/opam-repository#15795

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dune subst reads from dune-project #2955

dune subst reads from dune-project #2955

fangyi-zhou commented Dec 5, 2019

fangyi-zhou commented Dec 5, 2019

ghost Dec 10, 2019

ghost commented Dec 10, 2019

fangyi-zhou commented Dec 10, 2019

ghost commented Dec 11, 2019

ghost commented Dec 11, 2019

ghost commented Jan 15, 2020

fangyi-zhou commented Jan 15, 2020

ghost commented Jan 16, 2020

This comment has been minimized.

ghost commented Jan 16, 2020

ghost commented Jan 16, 2020 •

edited by ghost

fangyi-zhou commented Jan 16, 2020

dune subst reads from dune-project #2955

dune subst reads from dune-project #2955

Conversation

fangyi-zhou commented Dec 5, 2019

fangyi-zhou commented Dec 5, 2019

ghost Dec 10, 2019

Choose a reason for hiding this comment

ghost commented Dec 10, 2019

fangyi-zhou commented Dec 10, 2019

ghost commented Dec 11, 2019

ghost commented Dec 11, 2019

ghost commented Jan 15, 2020

fangyi-zhou commented Jan 15, 2020

ghost commented Jan 16, 2020

This comment has been minimized.

ghost commented Jan 16, 2020

ghost commented Jan 16, 2020 • edited by ghost

fangyi-zhou commented Jan 16, 2020

ghost commented Jan 16, 2020 •

edited by ghost