Relax restriction on project names being unique #2377
Conversation
rgrinberg
force-pushed
the
relax-duplicate-project-names
branch
from
9707c5d to
393de8e
Compare
|
Just to make sure we are not lifting this restriction too eagerly, given that it would be much harder to add it back in the future. What are our motivations for lifting it at this point? There was the virtual library problem, but it is now fixed in another way. |
The issue is that it makes composing duniverses less straightforward. It's not something that one can easily control when pulling in external code. By the way, I think by default we should be resist to introduce a uniqueness constraint on any kinds of user written tokens. In OCaml, one runs into problems with unique names not really being unique often enough we shouldn't be making this problem worse. |
That's true. However, that the same for package names and we generally encourage the two to be in sync.
Agreed. However, here we are removing this constraint rather than adding one :) Removing a constraint is much easier than adding one, so we must be sure that when we remove one, we really don't need it and we are not going to need it again. |
|
As we've previously experimented, we tried to make use of it for virtual libraries and later discovered that it was the wrong tool. Given that it's not a very good tool to solve problems, I'm fairly confident that we can avoid it :) |
|
Yh I suppose this information is a bit odd as project names appear nowhere else than where they are declared... What are the last two problematic cases then? Is it ppx and odoc? |
Yup. When one generates the html for a particular library, we need a unique name for the html dir of every private library. For ppx, I'm a bit confused about our current code. Are we still supporting the old .ppx layout for jbuild projects? If so, then why? It should be easier to just use one scheme. |
|
Yep, we are. The exact reason why has slipped my mind. I should definitely have written it down in the source code... Maybe it was just simpler to keep the old scheme to maintain backward compatibility. I remember that I tried to do something clever at the beginning and it wasn't working well. We can drop the old scheme in Dune 2.0. It should be possible to hack something in the upgrader only. In the meantime, what about simply adding a suffix when we have two projects with the same name in the tree? |
How do we map the suffix back to the project when decoding the keys however? |
|
We can use a separator character that we forbid in project names. |
rgrinberg
force-pushed
the
relax-duplicate-project-names
branch
from
393de8e to
4e67941
Compare
|
@diml see the latest commit which adjusts the preprocessing code not to use project names. i still need to adjust the old ppx pipeline + odoc |
|
Ok, I'll have a look in a minute. If the old ppx pipeline fails when two projects have the same name, that's fine. No need to make our life harder for things that have been deprecated for so long. Plus, this code will go away very soon |
rgrinberg
force-pushed
the
relax-duplicate-project-names
branch
from
4e67941 to
73b5102
Compare
|
Ah I already went through the effort of making it work. I moved all the legacy crap to the jbuild branch so that it will be easy to delete. |
|
Ok, well if we have the code we should consider it :) The main question is: can it introduce bugs when building jbuilder projects? If there is no risk, we might as well include the change. If there is a risk and this might cause us more work as a result, it doesn't seem worth including. |
|
Well if we don't include this then jbuild project that install ppx's aren't going to be build. I don't think there's much risk as its exactly the same code copy pasted. |
rgrinberg
force-pushed
the
relax-duplicate-project-names
branch
from
73b5102 to
c37da68
Compare
|
ok, let's include it then |
src/dune_project.ml
Outdated
|
|
||
| let of_project p = | ||
| String.take ( | ||
| Digest.generic (p.project_file, p.name, p.root) |
There was a problem hiding this comment.
project_file is redundant with name and root. It doesn't seem worth including both, we should keep only the name and root.
|
BTW, |
Project names no longer need to be unique. This requires us to change how serialize the html paths for odoc. We now need to include a token to disambuiguate projects with identical names. The old ppx code for jbuilder that still relies on project names remains broken. Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com> Improve type of Scope_key.to_string It only works with a library name Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com> [ppx] stop relying on the project name Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com> Fix odoc and the ppx pipeline to workaround project name collisions Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com> Make find_scope_by_name return a list of scopes Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com> Use file key to encode projects in ppx Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com> Include only the name and root in Dune_project.File_key Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com> Add doc to Dune_project.File_key Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com> Change Dune_project.File_key Make it a part of the Dune_project.t value. This avoids recompuatation. Convert the name/path to something more stable for serialization. Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com> Fix formatting Signed-off-by: Jeremie Dimino <jeremie@dimino.org> Digest paths directly Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
rgrinberg
force-pushed
the
relax-duplicate-project-names
branch
from
0c6b013 to
c323c96
Compare
CHANGES: - Don't select all local implementations in `dune utop`. Instead, let the default implementation selection do its job. (ocaml/dune#2327, fixes ocaml/dune#2323, @TheLortex, review by @rgrinberg) - Check that selected implementations (either by variants or default implementations) are indeed implementations. (ocaml/dune#2328, @TheLortex, review by @rgrinberg) - Don't reserve the `Ppx` toplevel module name for ppx rewriters (ocaml/dune#2242, @diml) - Redesign of the library variant feature according to the ocaml/dune#2134 proposal. The set of variants is now computed when the virtual library is installed. Introducing a new `external_variant` stanza. (ocaml/dune#2169, fixes ocaml/dune#2134, @TheLortex, review by @diml) - Add proper line directives when copying `.cc` and `.cxx` sources (ocaml/dune#2275, @rgrinberg) - Fix error message for missing C++ sources. The `.cc` extension was always ignored before. (ocaml/dune#2275, @rgrinberg) - Add `$ dune init project` subcommand to create project boilerplate according to a common template. (ocaml/dune#2185, fixes ocaml/dune#159, @shonfeder) - Allow to run inline tests in javascript with nodejs (ocaml/dune#2266, @hhugo) - Build `ppx.exe` as compiling host binary. (ocaml/dune#2286, fixes ocaml/dune#2252, @toots, review by @rgrinberg and @diml) - Add a `cinaps` extension and stanza for better integration with the [cinaps tool](https://github.com/janestreet/cinaps) tool (ocaml/dune#2269, @diml) - Allow to embed build info in executables such as version and list and version of statically linked libraries (ocaml/dune#2224, @diml) - Set version in `META` and `dune-package` files to the one read from the vcs when no other version is available (ocaml/dune#2224, @diml) - Add a variable `%{target}` to be used in situations where the context requires at most one word, so `%{targets}` can be confusing; stdout redirections and "-o" arguments of various tools are the main use case; also, introduce a separate field `target` that must be used instead of `targets` in those situations. (ocaml/dune#2341, @aalekseyev) - Fix dependency graph of wrapped_compat modules. Previously, the dependency on the user written entry module was omitted. (ocaml/dune#2305, @rgrinberg) - Allow to promote executables built with an `executable` stanza (ocaml/dune#2379, @diml) - When instantiating an implementation with a variant, make sure it matches virtual library's list of known implementations. (ocaml/dune#2361, fixes ocaml/dune#2322, @TheLortex, review by @rgrinberg) - Add a variable `%{ignoring_promoted_rules}` that is `true` when `--ingore-promoted-rules` is passed on the command line and false otherwise (ocaml/dune#2382, @diml) - Fix a bug in `future_syntax` where the characters `@` and `&` were not distinguished in the names of binding operators (`let@` was the same as `let&`) (ocaml/dune#2376, @aalekseyev, @diml) - Workspaces with non unique project names are now supported. (ocaml/dune#2377, fix ocaml/dune#2325, @rgrinberg) - Improve opam generation to include the `dune` dependncies with the minimum constraint set based on the dune language version specified in the `dune-project` file. (2383, @avsm) - The order of fields in the generated opam file now follows order preferred in opam-lib. (@avsm, ocaml/dune#2380) - Fix coloring of error messages from the compiler (@diml, ocaml/dune#2384) - Add warning `66` to default set of warnings starting for dune projects with language verison >= `1.11` (@rgrinberg, @diml, fixes ocaml/dune#2299) - Add (dialect ...) stanza (@nojb, ocaml/dune#2404) - Add a `--context` argument to `dune install/uninstall` (@diml, ocaml/dune#2412) - Do not warn about merlin files pre 1.9. This warning can only be disabled in 1.9 (ocaml/dune#2421, fixes ocaml/dune#2399, @emillon)
CHANGES: - Don't select all local implementations in `dune utop`. Instead, let the default implementation selection do its job. (ocaml/dune#2327, fixes ocaml/dune#2323, @TheLortex, review by @rgrinberg) - Check that selected implementations (either by variants or default implementations) are indeed implementations. (ocaml/dune#2328, @TheLortex, review by @rgrinberg) - Don't reserve the `Ppx` toplevel module name for ppx rewriters (ocaml/dune#2242, @diml) - Redesign of the library variant feature according to the ocaml/dune#2134 proposal. The set of variants is now computed when the virtual library is installed. Introducing a new `external_variant` stanza. (ocaml/dune#2169, fixes ocaml/dune#2134, @TheLortex, review by @diml) - Add proper line directives when copying `.cc` and `.cxx` sources (ocaml/dune#2275, @rgrinberg) - Fix error message for missing C++ sources. The `.cc` extension was always ignored before. (ocaml/dune#2275, @rgrinberg) - Add `$ dune init project` subcommand to create project boilerplate according to a common template. (ocaml/dune#2185, fixes ocaml/dune#159, @shonfeder) - Allow to run inline tests in javascript with nodejs (ocaml/dune#2266, @hhugo) - Build `ppx.exe` as compiling host binary. (ocaml/dune#2286, fixes ocaml/dune#2252, @toots, review by @rgrinberg and @diml) - Add a `cinaps` extension and stanza for better integration with the [cinaps tool](https://github.com/janestreet/cinaps) tool (ocaml/dune#2269, @diml) - Allow to embed build info in executables such as version and list and version of statically linked libraries (ocaml/dune#2224, @diml) - Set version in `META` and `dune-package` files to the one read from the vcs when no other version is available (ocaml/dune#2224, @diml) - Add a variable `%{target}` to be used in situations where the context requires at most one word, so `%{targets}` can be confusing; stdout redirections and "-o" arguments of various tools are the main use case; also, introduce a separate field `target` that must be used instead of `targets` in those situations. (ocaml/dune#2341, @aalekseyev) - Fix dependency graph of wrapped_compat modules. Previously, the dependency on the user written entry module was omitted. (ocaml/dune#2305, @rgrinberg) - Allow to promote executables built with an `executable` stanza (ocaml/dune#2379, @diml) - When instantiating an implementation with a variant, make sure it matches virtual library's list of known implementations. (ocaml/dune#2361, fixes ocaml/dune#2322, @TheLortex, review by @rgrinberg) - Add a variable `%{ignoring_promoted_rules}` that is `true` when `--ingore-promoted-rules` is passed on the command line and false otherwise (ocaml/dune#2382, @diml) - Fix a bug in `future_syntax` where the characters `@` and `&` were not distinguished in the names of binding operators (`let@` was the same as `let&`) (ocaml/dune#2376, @aalekseyev, @diml) - Workspaces with non unique project names are now supported. (ocaml/dune#2377, fix ocaml/dune#2325, @rgrinberg) - Improve opam generation to include the `dune` dependncies with the minimum constraint set based on the dune language version specified in the `dune-project` file. (2383, @avsm) - The order of fields in the generated opam file now follows order preferred in opam-lib. (@avsm, ocaml/dune#2380) - Fix coloring of error messages from the compiler (@diml, ocaml/dune#2384) - Add warning `66` to default set of warnings starting for dune projects with language verison >= `1.11` (@rgrinberg, @diml, fixes ocaml/dune#2299) - Add (dialect ...) stanza (@nojb, ocaml/dune#2404) - Add a `--context` argument to `dune install/uninstall` (@diml, ocaml/dune#2412) - Do not warn about merlin files pre 1.9. This warning can only be disabled in 1.9 (ocaml/dune#2421, fixes ocaml/dune#2399, @emillon) - Add a new `inline_tests` field in the env stanza to control inline_tests framework with a variable (ocaml/dune#2313, @mlasson, original idea by @diml, review by @rgrinberg). - New binary kind `js` for executables in order to explicitly enable Javascript targets, and a switch `(explicit_js_mode)` to require this mode in order to declare JS targets corresponding to executables. (ocaml/dune#1941, @nojb)
Project names no longer need to be unique, but they are still selected
randomly for compat ppx and odoc.
@diml I'm not entirely sure how to get rid of the last 2 use cases for project names. We still need to encode the scope in a unique way, but that's harder with non unique project names. Perhaps we need to some sort of digest based approach where we keep track of the reverse map? Similar to how you've handled the ppx map.