dub
dub copied to clipboard
dlang
WIP: refactor commands & environment variables
Environment variables are now consistent with custom command variables and are properly substituted at command run time to match with the recipe variables.
Additionally this fixes issue #2229 where sourceLibrary variables would get expanded later with wrong package dir, etc.
Might resolve issue #276 as well.
cc @rikkimax @veelo
What I am looking for in either this PR or a follow up is moving the variable substitution stuff out into it's own module.
It is fairly self contained logic but it is spread out over a few different files in random locations.
Also differentiating between early and just in time substitution might be a good thing too.
I'll review properly once I wake up (I really shouldn't be reviewing in bed on mobile! Lol).
On Tue, May 17, 2022, 02:17 Jan Jurzitza @.***> wrote:
@.**** commented on this pull request.
In source/dub/generators/generator.d https://github.com/dlang/dub/pull/2251#discussion_r873783408:
}
// propagate dependency environment variables to root// foreach (pack; m_project.getTopologicalPackageList(false, null, configs)) {// auto ti = pack.name in targets;// auto parentEnvs = pack.name in envs;// foreach (deppkgName, depInfo; pack.getDependencies(ti.config)) {// if (auto childEnvs = deppkgName in envs) {// childEnvs.update(ti.buildSettings);// parentEnvs.update(childEnvs);// }// }// }note: there was still some issue here
— Reply to this email directly, view it on GitHub https://github.com/dlang/dub/pull/2251#pullrequestreview-974038662, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHSL47UF5WCJFTMHORASZ3VKJKGFANCNFSM5WBUOVBA . You are receiving this because you were mentioned.Message ID: @.***>
Apart from acknowledging CI is failing and there is a bunch of code commented out, looking like it's going in the right direction.
I stand by what I said earlier about splitting out the variable substitution stuff entirely into its own module. It's all far too spread out yet so simple (in theory at least).
Do you have time to continue work on this, or do you want me to try and pick this up?
right now this rework broke support for inheriting set environment variables from dependencies, so I think that's all that needs to be fixed here. I will fix that myself, not sure if other things are missing right now.
I will fix that myself
Friendly ping @WebFreak001 to let you know that we are still held back by #2229.
WTF how do these changes break the sanitize function of std.encoding inside commandline.d?!
source/dub/commandline.d(440,51): Error: function `std.encoding.sanitize!char.sanitize` at /Users/runner/hostedtoolcache/dc/ldc2-1.30.0/x64/ldc2-1.30.0-osx-universal/bin/../import/std/encoding.d(1846,16) conflicts with function `std.encoding.sanitize!char.sanitize` at /Users/runner/hostedtoolcache/dc/ldc2-1.30.0/x64/ldc2-1.30.0-osx-universal/bin/../import/std/encoding.d(1846,16)
I actually just noticed there are libraries on DUB that already rely on the behavior that sourceLibrary environment variables would get inherited into the package. (well it's my own library, dorm, but I didn't think of this issue when I added the commands and it might be used elsewhere too) https://github.com/rorm-orm/dorm/blob/eeb575b36dc40778b3b509bdfeb9b1cf7924f141/build-models/dub.sdl
@Geod24 what do you think? Should we make sourceLibrary commands (preBuild/postBuild/preGenerate/postGenerate/preRun/postRun) expand variables from the context of the sourceLibrary package or in the context of the package that depends on the source lib?
Commands that are inherited only run inside the context of the package that depends on the source lib, because a source library does not have run or build stages.
@veelo how are you using the commands / why do you need the package path of the source library (and not change the source library to a regular library instead?)
We could also think about adding some $SOURCE_PACKAGE_PATH variable that's only valid in the context of source libraries, if we go with making the commands use the environment variables of the depender package.
Right now a workaround to add a dummy "library" package that's just there to run commands should work as well, if we decide to keep this behavior.
We shouldn't break people's packages where possible. But I do think it needs to be configurable via the map syntax that we talked about previously.
how about we throw away this PR and do the different command running syntax instead, adding a property to specify where the commands should be run? (although I can see that that might make the code flow a bit confusing)
So something like?
{
"command": [
"shell",
{
"shell": ["shells"],
"compileAndRun": ["file.d"],
"workingDirectory": "$EXEorSO_DIR",
"envSource": "this/exeOrSO",
"trigger": "default/always/never"
}
]
}
Thank you very much for picking this up again.
@veelo how are you using the commands / why do you need the package path of the source library (and not change the source library to a regular library instead?)
I use these commands to generate various (re)sources, like
"preGenerateCommands": [
"rdmd pvmake.d $PACKAGE_DIR/import"
],
"preBuildCommands": [
"cd $PACKAGE_DIR && rdmd tvmake.d $PACKAGE_DIR"
],
The wish to use these from a sourceLibrary package as opposed to a library package is admittedly unorthodox. We have around twenty programs that all use various modules from a large collection of shared sources. In order to not maintain 20 different dub.json files explicitly stating the various modules that each program depends on, we let the compiler figure out the dependencies using the -i option. Since Dub doesn't really support that, I had to find a work around. The work around is to have a package without sources and only build options, flags, preBuildCommands and of course importPaths. Dub complains if you try to configure a library without sources, so I had to make it a sourceLibrary. It kind of makes sense, because there is no artifact either from this package.
We could also think about adding some
$SOURCE_PACKAGE_PATHvariable that's only valid in the context of source libraries, if we go with making the commands use the environment variables of the depender package.
I guess that would work for me.
Right now a workaround to add a dummy "library" package that's just there to run commands should work as well, if we decide to keep this behavior.
That would require a dummy source file as well, and produce an artifact to link in.
Another example where we use postBuildCommands is
{
"name": "ctups",
"description": "Binding to Existec ctups",
"homepage": "https://existec.com/product/hazcheck-toolkits/",
"authors": [
"SARC"
],
"copyright": "Copyright © 2022, SARC B.V.",
"license": "proprietary",
"targetType": "sourceLibrary",
"targetPath": "$PACKAGE_DIR/../../bin",
"sourceFiles": [
"source/ctups.lib"
],
"copyFiles": [
"source/ctups.dll",
"source/subs.ind",
"source/libiomp5md.dll",
"source/frglobe.msg"
],
"comment": "https://github.com/dlang/dub/issues/2234",
"postBuildCommands-windows": [
"attrib -R %DUB_TARGET_PATH%/ctups.dll",
"attrib -R %DUB_TARGET_PATH%/subs.ind"
]
}
to change file attributes on 3rd party DLLs. This is a sourceLibrary because the only D source in it contains the header to the library, the .lib is supplied and should not be overwritten by this package.
Third example is similar to the second: a 3rd party lib where preGenerateCommands are used to run dpp:
{
"name": "wibu",
"description": "Protection implementation using the Wibu CodeMeter API.",
"authors": [
"Bastiaan N. Veelo"
],
"copyright": "Copyright © 2018, SARC B.V.",
"license": "proprietary",
"targetType": "sourceLibrary",
"libs": [
"$PACKAGE_DIR/WibuCm32"
],
"excludedSourceFiles": [
"source/make.d"
],
"preGenerateCommands": [
"cd $PACKAGE_DIR/source && rdmd make"
]
}
with make.d:
import std;
void main()
{
make(["sentinel.dpp"], ["sentinel.d"], ["dub", "run", "dpp", "--", "--preprocess-only", "sentinel.dpp"]);
}
void make(string[] input, string[] output, string[] args)
{
input ~= __FILE__;
if ((){
foreach (outp; output)
{
immutable outpTime = timeLastModified(outp, SysTime.min);
foreach (inp; input)
if (timeLastModified(inp) > outpTime)
return true;
}
return false;
}())
{
stderr.writeln(args.joiner(" "));
auto dmd = execute(args);
enforce(dmd.status == 0, "Compilation failed:\n" ~ dmd.output);
}
}
Is a different syntax going to solve anything? I still think there needs to be a variable for commands in a sourceLibrary to refer to its own package dir, irrespective of the syntax for the commands.
I actually just noticed there are libraries on DUB that already rely on the behavior that sourceLibrary environment variables would get inherited into the package. (well it's my own library, dorm, but I didn't think of this issue when I added the commands and it might be used elsewhere too) https://github.com/rorm-orm/dorm/blob/eeb575b36dc40778b3b509bdfeb9b1cf7924f141/build-models/dub.sdl
Pasting this in:
name "build-models"
targetType "sourceLibrary"
configuration "application" {
postBuildCommands ".\\$DUB_ROOT_PACKAGE_TARGET_PATH$DUB_ROOT_PACKAGE_TARGET_NAME --DORM-dump-models-json" platform="windows"
postBuildCommands "./$DUB_ROOT_PACKAGE_TARGET_PATH$DUB_ROOT_PACKAGE_TARGET_NAME --DORM-dump-models-json" platform="posix"
versions "DormBuildModels"
}
These all concern the root package, so I don't understand how these would expand differently. The root package would be the same irrespective of this package being a library or a sourceLibrary, no? Maybe I misunderstand what a root package is.
Is a different syntax going to solve anything? I still think there needs to be a variable for commands in a
sourceLibraryto refer to its own package dir, irrespective of the syntax for the commands.
Unfortunately, it would yes. Not all compilers support -run and rdmd is not always available. I personally am affected by commands triggering a rebuild and I want to disable that. Basically, the map would allow us to configure its behavior in more desirable ways and not be limited to using shell commands (which there might not be a shell!).
@veelo the code in dorm runs the app that has just being built (the root package of the user) with special flags that are handled by that dependency in a module constructor.
@WebFreak001 OK, but the meaning of root package should be the same, be it a library or sourceLibrary, right?
hmm unsure about root package, it's a quite random concept I think - what's the root package if your root package is sourceLibrary? I think in that case rootPackage should still refer to the package that is being built. (the depender)
If the introduction of $SOURCE_PACKAGE_PATH makes anything easier, I would be happy with that. At least it gives me a way out of #2229, so I can move on.
Right now a workaround to add a dummy "library" package that's just there to run commands should work as well, if we decide to keep this behavior.
That would require a dummy source file as well, and produce an artifact to link in.
For lack of an alternative, this is what I have started to resolve to.