Closed rtbo closed 1 year ago
github-actions[bot]
commented
1 year ago ✅ PR OK, no changes in deprecations or warnings
Total deprecations: 14
Total warnings: 0
Build statistics:
statistics (-before, +after)
executable size=5280256 bin/dub
-rough build time=77s
+rough build time=76s
rtbo
commented
1 year ago Instead of cacheBuildId, I could use cacheArtifactPath with the full path to the built artifact in the cache.
It would avoid Meson to take any assumption about the internal cache structure of Dub
i.e.
$dubhome/cache/$pkg/$version[/+$subpkg]/build/$buildId/$targetFileNameEven with the build Id available, if this structure changes, Meson will be lost again.
PetarKirov
commented
1 year ago A full path would be better, as Meson shouldn't depend on implementation details of Dub. As @Geod24 mentioned, dub describe is what other tools should use and if it doesn't contain all necessary information we should simply update it to fix that.
rtbo
commented
1 year ago There seem to be a bug in ProjectGenerator.configurePackages that prevent this PR to work as intended.
For example, when I run dub describe eventcore, the target taggedalgebraic get versions from target eventcore, despite having no dependency towards eventcore:
"versions": [
"EventcoreEpollDriver",
"Have_taggedalgebraic"
],However when I run dub build eventcore, taggedalgebraic is not built (because static libs) and when I run dub build taggedalgebraic, it is built without the EventcoreEpollDriver version.
It means that dub describe is currently describing targets that can't actually be built.
This come down to this piece of code, which looks wrong to me. I don't understand why dependencies need versions from their parents.
// 1. downwards inherits versions, debugVersions, and inheritable build settings
static void configureDependencies(const scope ref TargetInfo ti, TargetInfo[string] targets, size_t level = 0)
{
// do not use `visited` here as dependencies must inherit
// configurations from *all* of their parents
logDebug("%sConfigure dependencies of %s, deps:%(%s, %)", ' '.repeat(2 * level), ti.pack.name, ti.dependencies);
foreach (depname; ti.dependencies)
{
auto pti = &targets[depname];
mergeFromDependent(ti.buildSettings, pti.buildSettings);
configureDependencies(*pti, targets, level + 1);
}
}
configureDependencies(*roottarget, targets);
WebFreak001
commented
1 year ago This would be enough for Meson to locate Dub's artifacts. If you prefer to use this proposal, feel free to revert https://github.com/dlang/dub/pull/2642.
actually I think having both would be beneficial, as dub describe will only tell you about a single configuration, but you might want to know what everything in the folder is.
rtbo
commented
1 year ago When I completely remove the "downwards inherits versions etc." section from Dub's code, I still make valid builds. Do you know the purpose of this (dependencies receiving versions from their parents) ?
Right now dub build and dub describe are not fully compatible with each other.
rikkimax
commented
1 year ago Do you know the purpose of this (dependencies receiving versions from their parents) ?
It is used quite heavily, see bindbc projects that use it to specify the version of the library it needs to load.
Very useful.
rtbo
commented
1 year ago It is used quite heavily, see bindbc projects that use it to specify the version of the library it needs to load.
Ah yes, quite obvious.
Anyway this PR is ready. It will be fully useful if the --deep flag I've proposed is merged.
rtbo
commented
1 year ago Hi, can this be merged before next Dub release? The change seems to have consensus. I'd like to use it to fix Meson support for Dub dependencies.
Allow 3rd party programs (e.g. Meson) to locate artifacts in the cache. Following @Geod24 comment in #2642.
@WebFreak001 @Geod24 This would be enough for Meson to locate Dub's artifacts. If you prefer to use this proposal, feel free to revert #2642.