feat: introduce the @pkg-deps 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.

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

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 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 added 2 commits October 28, 2024 16:43

feat: add pkg-deps alias

8b4cc4e

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

test: pkg-deps build only the package deps

917e85f

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce the @pkg-deps alias #11046

feat: introduce the @pkg-deps 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


		$ . ./helpers.sh

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

feat: introduce the @pkg-deps alias #11046

Are you sure you want to change the base?

feat: introduce the @pkg-deps 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