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.

Sign up for GitHub

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

Jump to bottom

Fix sites installation with opam #4730

Merged
merged 15 commits into from
Jun 24, 2021
Merged

Fix sites installation with opam #4730

merged 15 commits into from
Jun 24, 2021

Conversation

bobot
Copy link
Collaborator

@bobot bobot commented Jun 14, 2021

Cherrypick from 2.9 (wrong way sorry) #4645

@bobot bobot force-pushed the fix_sites_installation_with_opam branch from c372233 to 1ebf02e Compare June 14, 2021 09:20
@ghost
Copy link

ghost commented Jun 14, 2021 •
edited by ghost
Loading

We already have --promote-install-files. Can we have --promote-install-files=no instead of --no-promote-install-files? We already have a lot of options, so it seems better to augment existing ones rather than add new similar ones.

FTR, we patched cmdliner in #4718 #4678 to support "optional parameters" properly. i.e. you can change --promote-install-files to take an optional parameter, and in this case you have to write --promote-install-files=<value> if you want to pass a parameter, so that there is no ambiguity.

@bobot
Copy link
Collaborator Author

bobot commented Jun 14, 2021

Can we have --promote-install-files=no instead of --no-promote-install-files?

Good idea, I will do it. @ejgallego I will have an additional little commit for 2.9. Is it okay?

@bobot
Copy link
Collaborator Author

bobot commented Jun 14, 2021

In fact, now that I remember, I did it this way because currently we can't supersede -p with --promote-install-files (there can be only one_of them). We talked about strengthening this code but not for 2.9. In any case allowing every boolean option to be overridden using --no- is useful and common (e.g wget). We can have that as a general rule (and automatic for every flag).

@ejgallego
Copy link
Collaborator

Can we have --promote-install-files=no instead of --no-promote-install-files?

Good idea, I will do it. @ejgallego I will have an additional little commit for 2.9. Is it okay?

No problem, thanks!

@ghost
Copy link

ghost commented Jun 14, 2021

Hmm, removing command line options is difficult since the cli is not versioned. We shouldn't introduce an option that we know we are going to remove.

In any case allowing every boolean option to be overridden using --no- is useful and common (e.g wget). We can have that as a general rule (and automatic for every flag).

Optional parameters seems better to me. We started using them for other options such as --watch and it is working pretty well. I don't understand why it wouldn't work here. Plus optional parameters are more versatile since they allows more states than just yes or no.

@bobot
Copy link
Collaborator Author

bobot commented Jun 15, 2021 •
edited
Loading

To do that I had to fix our alias option problem. So 72d774e handles correctly alias options. So it modifies Cmdliner (is it okay to modify our version before the modification is accepted in Cmdliner? @rgrinberg) . It also fixes #4632 . It is a little bigger than what I would have wanted for late commits but it is a good improvement.

@bobot bobot force-pushed the fix_sites_installation_with_opam branch 3 times, most recently from e2331e8 to f0486ae Compare June 15, 2021 15:39
@ghost
Copy link

ghost commented Jun 16, 2021

is it okay to modify our version before the modification is accepted in Cmdliner?

Yes, we have already cross that bridge. However, we have https://github.com/ocaml-dune/cmdliner. @rgrinberg, have you been pushing patches to the ocaml-dune/cmdliner fork and then updating the copy in vendor?

@bobot bobot force-pushed the fix_sites_installation_with_opam branch from 37da607 to 4c69713 Compare June 17, 2021 08:44
@ejgallego ejgallego added this to the 2.9 milestone Jun 18, 2021
@ejgallego
Copy link
Collaborator

I'm adding the 2.9 milestone because I understand bits of this will have to be backported too.

@rgrinberg
Copy link
Member

have you been pushing patches to the ocaml-dune/cmdliner fork and then updating the copy in vendor?

Yes. Patches to cmdliner should be pushed there and then updated in this repo.

@ghost
Copy link

ghost commented Jun 22, 2021

Ack. I pushed the small patch added in #4678 to github.com/ocaml-dune/cmdliner (/cc @aalekseyev).

@ejgallego ejgallego mentioned this pull request Jun 24, 2021
Closed
@bobot
Copy link
Collaborator Author

bobot commented Jun 24, 2021 •
edited
Loading

So I'm going to merge it. Thanks @jeremiedimino for the cherry-pick.

bobot added 10 commits June 24, 2021 15:57
@bobot
[Install/Uninstall] Use an enum instead of a string
9328235 
Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
[Site] generate_site_module fails with -p
1582055 
Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
[Sites] delay the computation of content of generate_sites_module
58ef72b 
        Fixes ocaml#4415

Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
[Install.Entry] Generalize the functions to Path.t
2205d66 
                instead of Path.Build.t

Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
[Artifact_substitution] Allow to test the presence of a placeholder
1bf1b36 
Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
[Sites] Fix installation through opam
88d8b0e 
        - Substitute the files in an intermediate directory if needed
        - Backward compatible

        Fixes ocaml#4198 ocaml#4212

Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
[Sites] Fix an error message for generate_sites_module
8e2ca90 
Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
[Sites] Better localisation of error
47f8943 
        Fixes ocaml#4325

Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
[Sites] add failing test with sites name which is an OCaml keyword
80c8424 
Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
[Sites] sanitize site name in generated modules
cc27c7c 
        a name matching "$ocaml_keyword_*" is converted into name^"_"

        Fixes ocaml#4219

Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
[Artifact_substitution] Wrap some channel opening into a fiber
0b5c74c 
     So that opening and closing be evaluated directly

Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
Add reproduction case for ocaml#4682
206e270 
Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
[Cmdliner] Add alias option
b6e467f 
           options that expand to others. Uses it for `--release` and `-p`.

           Fixes ocaml#4682 , the options are really used

Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
Replace --no-promote-install-files to --promote-install-files
003d53c 
Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
Disable ocamlformat for cmdliner
e15308a 
Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot bobot force-pushed the fix_sites_installation_with_opam branch from 4c69713 to e15308a Compare June 24, 2021 14:01
@bobot bobot merged commit cbe1df2 into ocaml:main Jun 24, 2021
@bobot bobot mentioned this pull request Jun 24, 2021
Merged
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jun 29, 2021
@ejgallego
[new release] dune-action-plugin, dune-glob, dune-site, dune-configur…
a13f842 
…ator, dune-private-libs, dune and dune-build-info (2.9.0)

CHANGES:

- Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon)

- Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb)

- Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos)

- Allow to set up merlin in a variant of the default context
  (ocaml/dune#4145, @TheLortex, @voodoos)

- Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon)

- Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01)

- Fix generation of merlin configuration when using `(include_subdirs
  unqualified)` on Windows (ocaml/dune#4745, @nojb)

- Fix bug for the install of Coq native files when using `(include_subdirs qualified)`
  (ocaml/dune#4753, @ejgallego)

- Allow users to specify install target directories for `doc` and
  `etc` sections. We add new options `--docdir` and `--etcdir` to both
  Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723,
  @ejgallego, thanks to @JasonGross for reporting this issue)

- Fix issue where Dune would ignore `(env ... (coq (flags ...)))`
  declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg)

- Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to
  the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego)

- Fix a bug where instrumentation flags would be added even if the
  instrumentatation was disabled (@nojb, ocaml/dune#4770)

- Fix ocaml/dune#4682: option `-p` takes now precedence on environement variable
  `DUNE_PROFILE` (ocaml/dune#4730, ocaml/dune#4774, @bobot, reported by @dra27 ocaml/dune#4632)

- Fix installation with opam of package with dune sites. The `.install` file is
  now produced by a local `dune install` during the build phase (ocaml/dune#4730, ocaml/dune#4645,
  @bobot, reported by @kit-ty-kate ocaml/dune#4198)

- Fix multiple issues in the sites feature (ocaml/dune#4730, ocaml/dune#4645 @bobot, reported by @Lelio-Brun
  ocaml/dune#4219, by @Kakadu ocaml/dune#4325, by @toots ocaml/dune#4415)
@ejgallego ejgallego mentioned this pull request Jun 29, 2021
Merged
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jul 1, 2021
@ejgallego
[new release] dune-action-plugin, dune-glob, dune-site, dune-configur…
96c14a5 
…ator, dune-private-libs, dune and dune-build-info (2.9.0)

CHANGES:

- Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon)

- Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb)

- Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos)

- Allow to set up merlin in a variant of the default context
  (ocaml/dune#4145, @TheLortex, @voodoos)

- Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon)

- Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01)

- Fix generation of merlin configuration when using `(include_subdirs
  unqualified)` on Windows (ocaml/dune#4745, @nojb)

- Fix bug for the install of Coq native files when using `(include_subdirs qualified)`
  (ocaml/dune#4753, @ejgallego)

- Allow users to specify install target directories for `doc` and
  `etc` sections. We add new options `--docdir` and `--etcdir` to both
  Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723,
  @ejgallego, thanks to @JasonGross for reporting this issue)

- Fix issue where Dune would ignore `(env ... (coq (flags ...)))`
  declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg)

- Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to
  the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego)

- Fix a bug where instrumentation flags would be added even if the
  instrumentatation was disabled (@nojb, ocaml/dune#4770)

- Fix ocaml/dune#4682: option `-p` takes now precedence on environement variable
  `DUNE_PROFILE` (ocaml/dune#4730, ocaml/dune#4774, @bobot, reported by @dra27 ocaml/dune#4632)

- Fix installation with opam of package with dune sites. The `.install` file is
  now produced by a local `dune install` during the build phase (ocaml/dune#4730, ocaml/dune#4645,
  @bobot, reported by @kit-ty-kate ocaml/dune#4198)

- Fix multiple issues in the sites feature (ocaml/dune#4730, ocaml/dune#4645 @bobot, reported by @Lelio-Brun
  ocaml/dune#4219, by @Kakadu ocaml/dune#4325, by @toots ocaml/dune#4415)
MisterDA added a commit to MisterDA/dune that referenced this pull request Jul 11, 2021
@MisterDA
Don't use subst --root in Opam files
0c64baf 
Introduced in Dune 2.9, Opam files are generated with this line:

```
build: [
  ["dune" "subst" "--root" "."] {dev}
```

However, `dune subst` doesn't have the `--root` option, which cause
the Opam installation of packages generated by Dune with `(lang 2.9)`
to fail.

Revert to the previous line.

This was added in commit 88d8b0e in
PR ocaml#4730.

Signed-off-by: Antonin Décimo <antonin.decimo@gmail.com>
@MisterDA MisterDA mentioned this pull request Jul 11, 2021
Merged
@bobot bobot mentioned this pull request Jul 19, 2021
Open
MisterDA added a commit to MisterDA/dune that referenced this pull request Jul 19, 2021
@MisterDA
Don't use subst --root in Opam files
87581c0 
Introduced in Dune 2.9, Opam files are generated with this line:

```
build: [
  ["dune" "subst" "--root" "."] {dev}
```

However, `dune subst` doesn't have the `--root` option, which cause
the Opam installation of packages generated by Dune with `(lang 2.9)`
to fail.

Revert to the previous line.

This was added in commit 88d8b0e in
PR ocaml#4730.

Signed-off-by: Antonin Décimo <antonin@tarides.com>
bobot pushed a commit that referenced this pull request Jul 19, 2021
@MisterDA @bobot
Don't use subst --root in Opam files
e5e963a 
Introduced in Dune 2.9, Opam files are generated with this line:

```
build: [
  ["dune" "subst" "--root" "."] {dev}
```

However, `dune subst` doesn't have the `--root` option, which cause
the Opam installation of packages generated by Dune with `(lang 2.9)`
to fail.

Revert to the previous line.

This was added in commit 88d8b0e in
PR #4730.

Signed-off-by: Antonin Décimo <antonin@tarides.com>
ejgallego pushed a commit to ejgallego/dune that referenced this pull request Sep 7, 2021
@MisterDA @ejgallego
Don't use subst --root in Opam files
14ae3d1 
Introduced in Dune 2.9, Opam files are generated with this line:

```
build: [
  ["dune" "subst" "--root" "."] {dev}
```

However, `dune subst` doesn't have the `--root` option, which cause
the Opam installation of packages generated by Dune with `(lang 2.9)`
to fail.

Revert to the previous line.

This was added in commit 88d8b0e in
PR ocaml#4730.

Signed-off-by: Antonin Décimo <antonin@tarides.com>
ejgallego pushed a commit to ejgallego/dune that referenced this pull request Sep 7, 2021
@MisterDA @ejgallego
Don't use subst --root in Opam files
e07a247 
Introduced in Dune 2.9, Opam files are generated with this line:

```
build: [
  ["dune" "subst" "--root" "."] {dev}
```

However, `dune subst` doesn't have the `--root` option, which cause
the Opam installation of packages generated by Dune with `(lang 2.9)`
to fail.

Revert to the previous line.

This was added in commit 88d8b0e in
PR ocaml#4730.

Signed-off-by: Antonin Décimo <antonin@tarides.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.9
Development

Successfully merging this pull request may close these issues.
3 participants
@bobot @ejgallego @rgrinberg