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.

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

dependencies/dub: First try to describe local project #13555

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

the-horo
Copy link
Contributor

The current approach of determining dub dependencies is by specifying a name and, optionally, a version. Dub will then be called to generate a json summary of the package and code in meson will parse that and extract relevant information. This can be insufficient because dub packages can provide multiple configurations for multiple use-cases, examples include providing a configuration for an executable and a configuration for a library. As a practical example, the dub package itself provides an application configuration and multiple library configurations, the json description of dub will, by default, be for the application configuration which will make dub as a library unusable in meson.

This can be solved without modifying the meson build interface by having dub describe the entire local project and collecting dependencies information from that. This way dub will generate information based on the project's 'dub.json' file, which is free to require dependencies in any way accepted by dub, by specifying configurations, by modifying compilation flags etc. This is all transparent to meson as dub's main purpose is to provide a path to the library file generated by the dependency in addition to other command-line arguments for the compiler.

This change will, however, require that projects that want to build with meson also provided a 'dub.json' file in which dependency information is recorded. Failure to do so will not break existing projects that didn't use a 'dub.json', but they will be limited to what the previous implementation offered. Projects that already have a 'dub.json' should be fine, so long as the file is valid and the information in it matches the one in 'meson.build'. For example for a 'dependency()' call in 'meson.build' that dependency must exist in 'dub.json', otherwise the call will now fail when it worked previously.

Using a 'dub.json' also has as a consequence that the version of the dependencies that are found are the ones specified in 'dub.selections.json', which can be helpful for projects that already provide a 'dub.json' in addition to 'meson.build' to de-duplicate code.

In terms of other code changes:

  • multiple version requirements for a dub dependency now work, though they can only be used when a 'dub.json' is present in which case the version of dependencies is already pinned by 'dub.selections.json'
  • the 'd/11 dub' test case has been changed to auto-generate the 'dub.json' config outside of the source directory, as the auto-generated file triggers warning when parsed by dub, which upsets the new code as the warnings interfere with the legitimate output.

Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few style nits here, I'm hoping that some of our resident D people might chime in on this from a functional point of view.

mesonbuild/dependencies/dub.py Outdated Show resolved Hide resolved
mesonbuild/dependencies/dub.py Outdated Show resolved Hide resolved
mesonbuild/dependencies/dub.py Outdated Show resolved Hide resolved
mesonbuild/dependencies/dub.py Outdated Show resolved Hide resolved
@thesamesam
Copy link
Collaborator

cc @denizzzka @rtbo

@denizzzka
Copy link

We in D already have dub.json as standard name of DUB package config file.

I think the file name like dub_meson.selections.json is better conveys the meaning of what is happening

Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds five new test cases. Is it possible we can test multiple things in a single test case?

Running full end-to-end tests is relatively slow and some of the CI jobs already take a pretty long time.

Comment on lines 17 to 19
run_command(dub_exe, 'build', '--deep', '--compiler', dc, '--arch', arch,
'--root', root,
check: true)
Copy link
Member

@eli-schwartz eli-schwartz Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like basically all of these new test cases are running this command, which I think is supposed to recursively build dependencies, and then the tests maybe never actually build the dub project itself because they just detect which dependency version got found, or something?

But don't we already have the built dependencies we need? If not, can we guarantee it is built in CI rather than potentially very slowly building dependencies on every CI run?

Copy link
Contributor Author

@the-horo the-horo Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds five new test cases. Is it possible we can test multiple things in a single test case?

Yeah, I think some can be merged.

It looks like basically all of these new test cases are running this command, which I think is supposed to recursively build dependencies, and then the tests maybe never actually build the dub project itself?

But don't we already have the built dependencies we need? If not, can we guarantee it is built in CI rather than potentially very slowly building dependencies on every CI run?

I did want to ask about this. I've written the commands in the meson files because I wanted to support people running the testsuite by themselves, without having to go through the CI script or reading test failures to find the line that tells them how to invoke dub to test dependencies. It is also way more manageable to keep track of which dependencies are needed / what versions / what compiler they require etc.

However, as you have said, this has the potential to build a lot of stuff during the tests but note that dub will properly (usually) cache the results so if they have been installed during CI they will not be rebuilt during the tests. This could be made a little better by having the new tests specify "targetType": "none" in their dub.json which will make the dub build command to only build the dependencies.

The usually above refers to dub forcefully rebuilding stuff if a change in configuration occurs so:

  • dub build dep (builds)
  • dub build dep (cached)
  • dub build dep --config foo (builds)
  • dub build dep --config foo (cached)
  • dub build dep (builds again even if cached)

Now, if the issue is the potential of building stuff outside of CI then yes, all the dependencies would need to be gathered and installed manually.

There is also another issue with installing them during the tests, there is the possibility of dub being called twice to build the same dependency and if it isn't present dub may get into a race-condition with the other process (it happened to me one).

From your tone it seems you desire to have as little building possibly being done during the tests. I think only test 19 needs to do actual building though it may be possible to change it not to require it. Is that what you suggest?

@the-horo
Copy link
Contributor Author

We in D already have dub.json as standard name of DUB package config file.

I think the file name like dub_meson.selections.json is better conveys the meaning of what is happening

dub.selections.json only contains information about the version of packages that have been pulled in, it doesn't describe how to build dependencies, or what configurations to use form them, which is what I need for my project. I just need to be able to depend on dub (as a library) which the current implementation doesn't allow.

It is possible to have a separate meson.dub.json or any other name for a full recipe file which is only used by meson. dub supports the --recipe switch for picking a non-default recipe thought it seems to hard fail if there is no dub.json present even though it doesn't use it.

If we assume a project that support building with both dub and meson will the 2 configuration files dub.json and meson.dub.json ever have different content (genuine question, I've only written 1 project in D and in my case the two files would be identical)? Do note that, in this case, if dub decides that it should update the selections file it will update dub.selections.json which may be shared with dub.json.

@denizzzka
Copy link

denizzzka commented Aug 20, 2024

It is possible to have a separate meson.dub.json or any other name for a full recipe file which is only used by meson. dub supports the --recipe switch for picking a non-default recipe thought it seems to hard fail if there is no dub.json present even though it doesn't use it.

When I wrote my previous comment I assumed that dub.json file mentioned in this PR have not same sense as in DUB packages.

In DUB to use DUB configurations it is just need to pass configuration via the command line or from users dub.json. That is, for Meson, we need passing something too. Maybe we have here lack of dependency() option related to D?

d_dep = dependency('19-this-package', method: 'dub', d_cfg: 'custom')

Or maybe something like .partial_dependency() should be implemented?

d_dep = dependency('19-this-package', method: 'dub')
custom_cfg_dep = d_dep.d_subconfig('custom')

@the-horo
Copy link
Contributor Author

In DUB to use DUB configurations it is just need to pass configuration via the command line or from users dub.json. That is, for Meson, we need passing something too. Maybe we have here lack of dependency() option related to D?

d_dep = dependency('19-this-package', method: 'dub', d_cfg: 'custom')

Yes, but that would change the semantics of dependency which I tried to avoid by having all the changes be internal. It should also be noted that this approach doesn't scale. Dub allows one to change the configuration of dependencies, it allows to add different compilation flags and whatever else will be added. Taking this into account it makes little sense, in my opinion, to support every way a dependency can be specified in dub.

Or maybe something like .partial_dependency() should be implemented?

d_dep = dependency('19-this-package', method: 'dub')
custom_cfg_dep = d_dep.d_subconfig('custom')

I didn't know about this but I don't think it's a good idea either. A criticism of dub is that it gathers more dependencies then it needs to leading to possible conflicts between packages and more things being pointlessly downloaded & analyzed. Perhaps it would be wise not to have meson fall into the same trap but I imagine there could be a way to implement what you suggest in a way that lazily resolves the dependency.

@denizzzka
Copy link

Dub allows one to change the configuration of dependencies, it allows to add different compilation flags and whatever else will be added. Taking this into account it makes little sense, in my opinion, to support every way a dependency can be specified in dub.

.partial_dependency() allows to change compilation flags

Dependencies configurations changing can be implemented in proposed .d_subconfig()

@denizzzka
Copy link

it allows to add different compilation flags

What does this affect? ​​After all, the package used must already be compiled at this moment

@the-horo
Copy link
Contributor Author

Dub allows one to change the configuration of dependencies, it allows to add different compilation flags and whatever else will be added. Taking this into account it makes little sense, in my opinion, to support every way a dependency can be specified in dub.

.partial_dependency() allows to change compilation flags

Dependencies configurations changing can be implemented in proposed .d_subconfig()

Yeah I think it can be implemented that way, have dependency return an object that represents a dub dependency with only the name field set and have each of those methods like d_subconfig create a new object starting from the previous and setting a property. Do that enough and you'll get the dependency you want, and the backing code shouldn't be that complicated.

it allows to add different compilation flags

What does this affect? ​​After all, the package used must already be compiled at this moment

It has to do with the caching dub does. It takes into account all build parameters which is why the code needs to specify --compiler --arch --build when invoking dub. What is also taken into account is the flags used to build it so, DFLAGS=-foo dub build x produces a different archive than dub build x. If the DFLAGS variable is not right then dub describe will point to a nonexistent file which will make meson fail to find the dependency.

As an example of why this is useful imagine:
DFLAGS=-Wno-error dub build urld
If you don't setup your meson project with: DFLAGS=-Wno-error meson setup build the dependency('urld') call will fail.

@the-horo the-horo force-pushed the dub-dep branch 2 times, most recently from cadfbf6 to 8b67cae Compare August 20, 2024 05:23
@the-horo
Copy link
Contributor Author

@eli-schwartz

I've reduced the number of tests: now there are only 2. There is only 1 build command during the tests which builds a local package so it shouldn't be resource intensive. I didn't find any good package online that is fast to build and provides 2 configurations, the first an executable and the second a library so I went with the local build. If, in install.sh, the path to the meson source dir is known one could do dub build <mandatory_args> --root <meson_src>/test cases/d/17 ... and remove that build command from the tests.

I've changed the installed libraries in CI to be exact versions since some of the new tests check for versions so I wanted it to be consistent.

I've hit this locally as well but I needed to compile dub packages with --arch x86_64 otherwise they wouldn't be found during the tests so I've added that argument to each dub build command. I've removed some of the flags added by #13550 as --build debug and --deep shouldn't affect the final build, the issue is, I think, only with --arch.

@the-horo
Copy link
Contributor Author

It appears that CI is failing. The d/11 test is working so building the dub packages with only --arch x86_64 is not the problem.

Of course, the test runs fine on my machine. I've tried using the docker image and debug from that but, for some reason, dub decides to consume 6GB of memory when it invokes the compiler to determine its version and other flags and OOMs. This has happened to me both when trying to build the image manually and when using the prebuilt mesonbuild images. Its probably something broken with my docker but I still can't reproduce the failure. I even tried to recompile dub to exclude those problematic compiler calls and of course the tests pass in the docker image in that case as well.

The only thing I can think off after this is shove prints in the code to see why the tests fail. I love CI.

@the-horo
Copy link
Contributor Author

I'm getting ahead of myself. The CI images used in the tests are from previous runs so my changes to them don't reflect here. Not sure what to do about it though.

@eli-schwartz
Copy link
Member

Of course, the test runs fine on my machine. I've tried using the docker image and debug from that but, for some reason, dub decides to consume 6GB of memory when it invokes the compiler to determine its version and other flags and OOMs. This has happened to me both when trying to build the image manually and when using the prebuilt mesonbuild images. Its probably something broken with my docker but I still can't reproduce the failure. I even tried to recompile dub to exclude those problematic compiler calls and of course the tests pass in the docker image in that case as well.

This is exactly the same reason I ragequit attempting to locally debug D issues for the CI. You are not alone and it's not your docker, because I have the same problem.

If I try to build the test images, dub build consumes all the memory available to docker, which usually means OOMing my entire computer and breaking everything I was working on willy-nilly. So what I did was just give up and locally comment out all Dub handling when I test other issues on the same CI container. ;)

I never figured out the reason why.

@eli-schwartz
Copy link
Member

I'm getting ahead of myself. The CI images used in the tests are from previous runs so my changes to them don't reflect here. Not sure what to do about it though.

When updating the CI image container for Ubuntu, it is okay if the os_comp tests fail due to the out of date container as long as the container succeeds.

However, the Azure CI for Windows is also failing in this PR for the same reason.

@denizzzka
Copy link

Of course, the test runs fine on my machine. I've tried using the docker image and debug from that but, for some reason, dub decides to consume 6GB of memory

Try to run dub with option --DRT-gcopt=heapSizeFactor:1.1

@the-horo
Copy link
Contributor Author

the-horo commented Aug 21, 2024

Of course, the test runs fine on my machine. I've tried using the docker image and debug from that but, for some reason, dub decides to consume 6GB of memory when it invokes the compiler to determine its version and other flags and OOMs. This has happened to me both when trying to build the image manually and when using the prebuilt mesonbuild images. Its probably something broken with my docker but I still can't reproduce the failure. I even tried to recompile dub to exclude those problematic compiler calls and of course the tests pass in the docker image in that case as well.

This is exactly the same reason I ragequit attempting to locally debug D issues for the CI. You are not alone and it's not your docker, because I have the same problem.

If I try to build the test images, dub build consumes all the memory available to docker, which usually means OOMing my entire computer and breaking everything I was working on willy-nilly. So what I did was just give up and locally comment out all Dub handling when I test other issues on the same CI container. ;)

At least mine doesn't crash, just gracefully stops execution.

I never figured out the reason why.

The problematic lines in dub are the ones that call execute. Specifically I had to change https://github.com/dlang/dub/blob/665173f165649b18fe0829a16dc7f319a59a4d65/source/dub/compilers/gdc.d#L59-L63 and https://github.com/dlang/dub/blob/665173f165649b18fe0829a16dc7f319a59a4d65/source/dub/compilers/compiler.d#L187 to get dub to behave.

More problematic this code also OOMs:

import std.process : execute;

void main () {
	auto r = execute(["/usr/bin/true"], maxOutput: 1000);
	import std.stdio;
	writeln("Exit status: ", r.status);
	writeln("Output: ", [ r.output ]);
}

So it would seem D's execute is misbehaving inside docker. Note that here OOMs means that the execute process gets stopped though the parent continues execution:

root@f7400b61a0bd:/# gdc a.d  && ./a.out
Exit status: -9
Output: [""]

Try to run dub with option --DRT-gcopt=heapSizeFactor:1.1

No change:

root@f7400b61a0bd:/# gdc a.d  && ./a.out  --DRT-gcopt=heapSizeFactor:1.1
Exit status: -9
Output: [""]

I'll see if I can find what's going wrong here hopefully before the CI starts to fail as well.

@the-horo
Copy link
Contributor Author

I'm getting ahead of myself. The CI images used in the tests are from previous runs so my changes to them don't reflect here. Not sure what to do about it though.

When updating the CI image container for Ubuntu, it is okay if the os_comp tests fail due to the out of date container as long as the container succeeds.

However, the Azure CI for Windows is also failing in this PR for the same reason.

Yes, I have to change what dub packages need to be installed for windows as well.

the-horo added a commit to the-horo/cidata that referenced this pull request Aug 21, 2024
Mirror what mesonbuild/meson#13555 installs
for dub.

Signed-off-by: Andrei Horodniceanu <[email protected]>
the-horo added a commit to the-horo/cidata that referenced this pull request Aug 21, 2024
Mirror what mesonbuild/meson#13555 installs
for dub.

Signed-off-by: Andrei Horodniceanu <[email protected]>
jpakkane pushed a commit to mesonbuild/cidata that referenced this pull request Aug 21, 2024
Mirror what mesonbuild/meson#13555 installs
for dub.

Signed-off-by: Andrei Horodniceanu <[email protected]>
@the-horo
Copy link
Contributor Author

I've made a PR (dlang/dub#2962) to fix dub not finding dependencies if they aren't built with --arch. This should stop the issues of dub not finding the packages during the tests and it should be sufficient to do dub build --comp=... <pkg> when building the CI images.

@rtbo
Copy link
Contributor

rtbo commented Aug 24, 2024

relate to #10189 where I proposed to add configuration kwarg to dependency

vibe_http_dep = dependency('vibe-d:http',
    method: 'dub',
    configuration: 'vibe-d:tls/notls',
)

The current approach of determining dub dependencies is by specifying
a name and, optionally, a version. Dub will then be called to generate
a json summary of the package and code in meson will parse that and
extract relevant information. This can be insufficient because dub
packages can provide multiple configurations for multiple use-cases,
examples include providing a configuration for an executable and a
configuration for a library. As a practical example, the dub package
itself provides an application configuration and multiple library
configurations, the json description of dub will, by default, be for
the application configuration which will make dub as a library
unusable in meson.

This can be solved without modifying the meson build interface by
having dub describe the entire local project and collecting
dependencies information from that. This way dub will generate
information based on the project's 'dub.json' file, which is free to
require dependencies in any way accepted by dub, by specifying
configurations, by modifying compilation flags etc. This is all
transparent to meson as dub's main purpose is to provide a path to the
library file generated by the dependency in addition to other
command-line arguments for the compiler.

This change will, however, require that projects that want to build
with meson also provided a 'dub.json' file in which dependency
information is recorded. Failure to do so will not break existing
projects that didn't use a 'dub.json', but they will be limited to
what the previous implementation offered. Projects that already have a
'dub.json' should be fine, so long as the file is valid and the
information in it matches the one in 'meson.build'. For example for a
'dependency()' call in 'meson.build' that dependency must exist in
'dub.json', otherwise the call will now fail when it worked
previously.

Using a 'dub.json' also has as a consequence that the version of the
dependencies that are found are the ones specified in
'dub.selections.json', which can be helpful for projects that already
provide a 'dub.json' in addition to 'meson.build' to de-duplicate code.

In terms of other code changes:
- multiple version requirements for a dub dependency now work, though
they can only be used when a 'dub.json' is present in which case the
version of dependencies is already pinned by 'dub.selections.json'
- the 'd/11 dub' test case has been changed to auto-generate the
'dub.json' config outside of the source directory, as the
auto-generated file triggers warning when parsed by dub, which upsets
the new code as the warnings interfere with the legitimate output.

Signed-off-by: Andrei Horodniceanu <[email protected]>
@the-horo
Copy link
Contributor Author

Rebased for #13629. This should make the windows tests pass but they should still fail on linux.

@the-horo
Copy link
Contributor Author

Uh, the opensuse job should have failed. Trying to fix in #13673

@the-horo
Copy link
Contributor Author

@denizzzka @rtbo @eli-schwartz any more news on this? I'd appreciate if it would be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants