feat: introduce the @pkg-install alias #11046

maiste · 2024-10-28T14:41:49Z

This PR solves one part of #10949. It introduces a global alias with the source deps as a dependency. When you use it, you will automatically build the source deps of the project. Regarding the PR, I have various questions:

Should I update the documentation as it is related to package management? (I know we don't want to "pollute" the documentation with not released part)
Is there a better way to declare an alias, or is this way fine? I tried to take inspiration from other aliases.
For the test, would there be a cleaner way to check we only installed what we want at each stage. Here, it seems we can't use the printing solution, as we need to install a complete package.

Leonidas-from-XIV

I am unsure what our policy is about looking into _build in tests, but I have some ideas how to avoid doing it.

Leonidas-from-XIV · 2024-10-28T16:15:39Z

test/blackbox-tests/test-cases/pkg/build-pkg-deps.t

+  $ dune build @pkg-deps
+  $ ls _build/_private/default/.pkg/
+  foo
+  $ ls _build/default/


I was thinking about the way to detect that the build is happening is to maybe enable the console output and have the opam file echo "foo is building".

Why should there be a build happening? This alias is supposed to only download the deps.

Leonidas-from-XIV · 2024-10-28T16:16:23Z

test/blackbox-tests/test-cases/pkg/build-pkg-deps.t

+Verify that building the `bar.exe` file is working:
+  $ dune build ./bar.exe
+  $ ls _build/default/bar.exe
+  _build/default/bar.exe


And this could then just be replaced by dune exec ./bar.exe?

This has the slight downside that bar.exe could've been built before so maybe building bar.exe should also echo something to stdout?

It would mean to add a rule that depends on the bar.exe no?

dune exec ./bar.exe automatically adds a dependency. Unless I misunderstand your question.

rgrinberg · 2024-10-29T21:41:36Z

test/blackbox-tests/test-cases/pkg/build-pkg-deps.t

+  >  (depends foo))
+  > EOF
+  $ add_mock_repo_if_needed
+  $ dune pkg lock


Why do you need to involve the solver in this test?

It was the easiest way to write the test. However, it should be possible to write it without using the lock command. I'll rework it.

rgrinberg · 2024-10-29T21:44:01Z

test/blackbox-tests/test-cases/pkg/build-pkg-deps.t

+
+  $ . ./helpers.sh
+
+Create a fake library to install in the target project as a dependency:


I didn't really get why you need a fake library and executable in this test. I see the following as sufficient:

Write a lock directory

Build your new alias

Verify that the sources exist

Everything else seems redundant.

As we want to build the library and ensure it is installed, you need the fake library to make sure dune build correctly. Otherwise, it will fail when you try to use it as a dependency.

rgrinberg · 2024-10-29T21:51:17Z

To be consistent with dune, aliases should be attached to source directories. In particular:

The following should build all the sources in dune.lock

dune build @dune.lock/pkg-deps

The following should build all the sources in the workspace:

dune build @pkg-deps

If you'd like to download the sources for a particular package, maybe you could add an alias like dune build @dune.lock/$pkg-deps

rgrinberg · 2024-10-29T21:51:50Z

src/dune_rules/alias0.ml

@@ -25,4 +25,5 @@ let check = standard "check"
 let install = standard "install"
 let ocaml_index = standard "ocaml-index"
 let runtest = standard "runtest"
+let pkg_deps = standard "pkg-deps"


The point of this aliases is to download the sources? If it is, a more suggestive name would be pkg-sources or even just sources

maiste · 2024-10-30T09:30:42Z

@rgrinberg, I feel there is a misunderstanding with the purpose of this PR... The goal is to introduce an alias that allows to download and build the sources of a project. In the issue related to it, we discuss two aliases with two different purposes:

@pkg-fetch whose role is to download the sources without building them. The use case targetted here is when you know you are going to be offline.
@pkg-deps whose role is to download the source and build them. Doing this allow to add a caching layer with Docker when building image. Docker can cache the build dependencies. Hence, when you update your own code, the engine won't try to rebuild the source. This is what this PR is trying to do.

The following should build all the sources in dune.lock
dune build @dune.lock/pkg-deps

I see. Where would be the best part to add the alias? gen_rules.ml? Also, could I take inspiration from runtest? I feel this is pretty related to this mechanism.

If you'd like to download the sources for a particular package, maybe you could add an alias like dune build @dune.lock/$pkg-deps

Isn't it something that should be attached to @pkg-fetch as it seems more relevant?

rgrinberg · 2024-10-30T11:47:21Z

Okay, I think got confused by your implementation. It's currently adding the source deps to the alias. If you'd like the package build to included in the alias, you need to add the target directory of the package as a dependency.

rgrinberg · 2024-10-30T11:52:09Z

I see. Where would be the best part to add the alias? gen_rules.ml? Also, could I take inspiration from runtest? I feel this is pretty related to this mechanism.

pkg_rules would be the ideal place

Isn't it something that should be attached to @pkg-fetch as it seems more relevant?

Don't think there's any difference between fetching and building here. But if you decide that you don't need per package granularity, that's OK too.

Also, if you want to check that a package has been built, inspecting _build or using of the shell commands in helpers.sh seems like the better bet. Including extra libraries or executables is still unnecessary moving parts. Our tests suffer from being slow, redundant, and rather hard to understand when failing. Less is more here.

maiste · 2024-11-06T17:54:27Z

@rgrinberg, I tried to follow the advises given during the Dune Dev Meeting. However, there are still some parts I don't understand or misunderstood. I push my broken code to have a support to discuss on (I didn't rewrite the test yet).

As you can see in pkg_rules, I added a new rule that builds the target paths from the package universe and adds them as a dependency to the alias @pkg-install. Then, Dune register this rule as a dependency in gen_rules.ml as it was done for the other aliases. However, I encountered two errors:

I remember you told me to attach this rule to the source directory, not the _build directory. Unfortunately, I'm not able to find a way to do it in the current situation. If I try to attach the alias to project.root (not in this version of the code but on my machine), I get an error that I must not do this. What is wrong with the way I have done it here? In this version, it keeps telling me it is not attached:
```
Error: Alias "pkg-install" specified on the command line is empty.
It is not defined in . or any of its descendants.
```
How can I attach it to the workspace?
My first version was trying to register this rule for all the packages, including the one not using package management. This is fixed by the last tmp commit I pushed.

Otherwise, was it similar to the approach you had in mind?

Signed-off-by: Etienne Marais <[email protected]>

Attached it to the workspace. Signed-off-by: Etienne Marais <[email protected]>

Signed-off-by: Etienne Marais <[email protected]>

rgrinberg · 2024-11-13T11:39:33Z

src/dune_rules/pkg_rules.ml

+    let pkg_name = Dune_lang.Package_name.to_string pkg in
+    Path.Build.L.relative
+      Private_context.t.build_dir
+      [ Context_name.to_string ctx_name; ".pkg"; pkg_name; "target" ]


You should go through the paths module to get this path

rgrinberg · 2024-11-13T11:40:31Z

src/dune_rules/alias0.ml

@@ -25,4 +25,5 @@ let check = standard "check"
 let install = standard "install"
 let ocaml_index = standard "ocaml-index"
 let runtest = standard "runtest"
+let pkg_install = standard "pkg-install"


It's incorrect to mark this alias as "standard" as it's not defined in every single directory.

If I don't mark it as standard, it will complain that it is not attached to any folder in the source. What is the best way to attached it to the source if I don't declare it as standard?

rgrinberg · 2024-11-13T11:41:20Z

src/dune_rules/gen_rules.ml

@@ -355,6 +368,7 @@ let gen_project_rules =
    and+ () = Odoc.gen_project_rules sctx project
    and+ () = Odoc_new.gen_project_rules sctx project
    and+ () = Ocaml_index.project_rule sctx project
+    and+ () = gen_project_rule_for_package_alias sctx project


This needs to be done only once per context rather than for every project

maiste added the package management label Oct 28, 2024

maiste requested review from Leonidas-from-XIV, rgrinberg and moyodiallo October 28, 2024 14:41

maiste mentioned this pull request Oct 28, 2024

[pkg] fetch dependencies without building the project #10949

Open

maiste force-pushed the feat/pkg-deps-alias branch from ee81c9f to 917e85f Compare October 28, 2024 15:44

Leonidas-from-XIV reviewed Oct 28, 2024

View reviewed changes

rgrinberg reviewed Oct 29, 2024

View reviewed changes

maiste force-pushed the feat/pkg-deps-alias branch from 917e85f to 3393cb5 Compare November 6, 2024 17:29

Leonidas-from-XIV mentioned this pull request Nov 7, 2024

pkg: Install all the dev-tools that are available #11032

Open

maiste added 3 commits November 12, 2024 11:37

feat: add pkg-deps alias

6397559

Signed-off-by: Etienne Marais <[email protected]>

test: pkg-deps build only the package deps

975741e

Signed-off-by: Etienne Marais <[email protected]>

feat: use pkg-install to build the deps

877dbbe

Attached it to the workspace. Signed-off-by: Etienne Marais <[email protected]>

maiste force-pushed the feat/pkg-deps-alias branch from 6849873 to 877dbbe Compare November 12, 2024 14:51

maiste changed the title ~~feat: introduce the @pkg-deps alias~~ feat: introduce the @pkg-install alias Nov 12, 2024

fix: clean some part of the code

0f02cc5

Signed-off-by: Etienne Marais <[email protected]>

rgrinberg requested changes Nov 13, 2024

View reviewed changes

rgrinberg reviewed Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce the @pkg-install alias #11046

feat: introduce the @pkg-install alias #11046

maiste commented Oct 28, 2024

Leonidas-from-XIV left a comment

Leonidas-from-XIV Oct 28, 2024

rgrinberg Oct 29, 2024

Leonidas-from-XIV Oct 28, 2024

maiste Oct 28, 2024

Leonidas-from-XIV Oct 30, 2024

rgrinberg Oct 29, 2024

maiste Oct 30, 2024

rgrinberg Oct 29, 2024

maiste Oct 30, 2024

rgrinberg commented Oct 29, 2024

rgrinberg Oct 29, 2024

maiste commented Oct 30, 2024

rgrinberg commented Oct 30, 2024

rgrinberg commented Oct 30, 2024

maiste commented Nov 6, 2024

rgrinberg Nov 13, 2024

rgrinberg Nov 13, 2024

maiste Nov 13, 2024

rgrinberg Nov 13, 2024


		$ . ./helpers.sh

		Create a fake library to install in the target project as a dependency:

feat: introduce the @pkg-install alias #11046

Are you sure you want to change the base?

feat: introduce the @pkg-install alias #11046

Conversation

maiste commented Oct 28, 2024

Leonidas-from-XIV left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Oct 29, 2024

Choose a reason for hiding this comment

maiste commented Oct 30, 2024

rgrinberg commented Oct 30, 2024

rgrinberg commented Oct 30, 2024

maiste commented Nov 6, 2024

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