WIP: refactor commands & environment variables #2251

WebFreak001 · 2022-05-16T14:14:28Z

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

WebFreak001 · 2022-05-16T14:16:53Z

source/dub/generators/generator.d

+		// 			parentEnvs.update(childEnvs);
+		// 		}
+		// 	}
+		// }


note: there was still some issue here

rikkimax · 2022-05-16T14:28:36Z

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 <#2251 (comment)>: > } + + // 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 <#2251 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHSL47UF5WCJFTMHORASZ3VKJKGFANCNFSM5WBUOVBA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

veelo · 2022-05-16T14:40:26Z

I really appreciate the work on this!

rikkimax · 2022-05-16T17:48:56Z

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).

veelo · 2022-05-23T11:27:06Z

Do you have time to continue work on this, or do you want me to try and pick this up?

WebFreak001 · 2022-05-23T12:25:32Z

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.

veelo · 2022-05-23T12:27:54Z

Alright thank you, let me know if I can help in any way.

veelo · 2022-07-08T14:48:56Z

I will fix that myself

Friendly ping @WebFreak001 to let you know that we are still held back by #2229.

WebFreak001 · 2023-01-08T19:08:33Z

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)

WebFreak001 · 2023-01-08T19:18:46Z

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.

WebFreak001 · 2023-01-08T19:20:50Z

@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.

rikkimax · 2023-01-08T19:24:08Z

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.

WebFreak001 · 2023-01-08T19:26:04Z

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)

rikkimax · 2023-01-08T19:31:42Z

So something like?

{
    "command": [
        "shell",
        {
             "shell": ["shells"],
             "compileAndRun": ["file.d"],
             "workingDirectory": "$EXEorSO_DIR",
             "envSource": "this/exeOrSO",
             "trigger": "default/always/never"
        }
    ]
}

veelo · 2023-01-08T20:06:46Z

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_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.

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);
  }
}

veelo · 2023-01-08T20:33:50Z

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.

veelo · 2023-01-08T20:39:50Z

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.

rikkimax · 2023-01-08T20:44:20Z

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.

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!).

WebFreak001 · 2023-01-08T20:52:48Z

@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.

veelo · 2023-01-08T20:59:41Z

@WebFreak001 OK, but the meaning of root package should be the same, be it a library or sourceLibrary, right?

WebFreak001 · 2023-01-08T21:01:33Z

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)

veelo · 2023-01-08T21:18:02Z

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.

veelo · 2023-01-19T09:58:50Z

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.

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 dlang#2229 where sourceLibrary variables would get expanded later with wrong package dir, etc.

WebFreak001 commented May 16, 2022

View reviewed changes

source/dub/generators/generator.d

// parentEnvs.update(childEnvs);

// }

// }

// }

Copy link

Member Author

WebFreak001 May 16, 2022

Choose a reason for hiding this comment

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

note: there was still some issue here

WebFreak001 force-pushed the fix-2229 branch from a616de4 to d8834a6 Compare March 2, 2023 00:31

WebFreak001 force-pushed the fix-2229 branch from d8834a6 to 868b922 Compare March 2, 2023 00:43

veelo mentioned this pull request May 21, 2023

[REG] $PACKAGE_DIR broken for sourceLibrary packages. #2229

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: refactor commands & environment variables #2251

WIP: refactor commands & environment variables #2251

WebFreak001 commented May 16, 2022 •

edited

Loading

WebFreak001 May 16, 2022

rikkimax commented May 16, 2022 via email

veelo commented May 16, 2022

rikkimax commented May 16, 2022

veelo commented May 23, 2022

WebFreak001 commented May 23, 2022

veelo commented May 23, 2022

veelo commented Jul 8, 2022

WebFreak001 commented Jan 8, 2023

WebFreak001 commented Jan 8, 2023

WebFreak001 commented Jan 8, 2023 •

edited

Loading

rikkimax commented Jan 8, 2023

WebFreak001 commented Jan 8, 2023

rikkimax commented Jan 8, 2023

veelo commented Jan 8, 2023

veelo commented Jan 8, 2023

veelo commented Jan 8, 2023

rikkimax commented Jan 8, 2023

WebFreak001 commented Jan 8, 2023

veelo commented Jan 8, 2023

WebFreak001 commented Jan 8, 2023

veelo commented Jan 8, 2023

veelo commented Jan 19, 2023

WIP: refactor commands & environment variables #2251

Are you sure you want to change the base?

WIP: refactor commands & environment variables #2251

Conversation

WebFreak001 commented May 16, 2022 • edited Loading

WebFreak001 May 16, 2022

Choose a reason for hiding this comment

rikkimax commented May 16, 2022 via email

veelo commented May 16, 2022

rikkimax commented May 16, 2022

veelo commented May 23, 2022

WebFreak001 commented May 23, 2022

veelo commented May 23, 2022

veelo commented Jul 8, 2022

WebFreak001 commented Jan 8, 2023

WebFreak001 commented Jan 8, 2023

WebFreak001 commented Jan 8, 2023 • edited Loading

rikkimax commented Jan 8, 2023

WebFreak001 commented Jan 8, 2023

rikkimax commented Jan 8, 2023

veelo commented Jan 8, 2023

veelo commented Jan 8, 2023

veelo commented Jan 8, 2023

rikkimax commented Jan 8, 2023

WebFreak001 commented Jan 8, 2023

veelo commented Jan 8, 2023

WebFreak001 commented Jan 8, 2023

veelo commented Jan 8, 2023

veelo commented Jan 19, 2023

WebFreak001 commented May 16, 2022 •

edited

Loading

WebFreak001 commented Jan 8, 2023 •

edited

Loading