[REG] $PACKAGE_DIR broken for sourceLibrary packages.

[veelo]

System information

• dub version: HEAD
• OS Platform and distribution: Windows 10
• compiler version v2.098.0

Bug Description

Since cb290e1 in #2217 $PACKAGE_DIR inside preBuildCommands of a sourceLibrary dependency returns the name of the package that has the dependency, not the name that is in the dub.json.

How to reproduce?

sub1\source\app.d:

void main() {}

sub1\dub.json:

{
	"name": "sub1",
	"targetType": "executable",
	"dependencies": {
		"sub2": {
			"path": "../sub2",
			"version": "*"
		}
	},
	"preBuildCommands": [
		"echo sub1 $$PACKAGE_DIR = $PACKAGE_DIR"
	]
}

sub2\dub.json:

{
	"name": "sub2",
	"targetType": "sourceLibrary",
	"preBuildCommands": [
		"echo sub2 $$PACKAGE_DIR = $PACKAGE_DIR"
	]
}

This produces the following output:

Running pre-build commands...
sub1 $PACKAGE_DIR = [path]\sub1
sub2 $PACKAGE_DIR = [path]\sub1

Expected Behavior

This should be the output:

Running pre-build commands...
sub1 $PACKAGE_DIR = [path]\sub1
sub2 $PACKAGE_DIR = [path]\sub2

[WebFreak001]

hm according to the dub documentation the $PACKAGE_DIR shouldn't exist in commands at all:

Inside of custom command directives a different set of predefined variables is available: [...]

[veelo]

I have been using $PACKAGE_DIR in places where I should have been using the other variables, but I didn't because I couldn't get them to work, due to the bug that you fixed in #2217. I will investigate today whether I can work without $PACKAGE_DIR in those commands, but chances are that I am not alone in having done this. It is a difficult situation because it is a breaking change in practice, and it is not something that can be given a deprecation period I would think.

The documentation you referenced doesn't speak with confidence to me: the first two sections start with exactly the same words, "Inside of build setting values..." so what is the difference between those sections? The custom command directives are in the Build Settings table and in those two sections there is nothing that says that the commands are excluded. It is only the prelude to the last table that says "different set", so one has to read past the information that says you can use the variable in commands to learn that you can't. I just checked and that sentence used to read as "Inside of custom commands directives a number of additional variables is available:" (emphasis added). It was changed by @pbackus in dlang/dub-docs@58d8fa3.

If we decide that the variables in the first two tables really should not be used in command directives then they should not be defined for them at all, and an error should be produced if you try to use them like that. And, this should be announced in big letters in the changelog, IMO. I just hope I am the only one that runs into this problem...

[veelo]

I will investigate today whether I can work without $PACKAGE_DIR in those commands

I don't think I can. If you have commands in a sourceLibrary (which obviously is a dependency) the current working directory is not in that package, but in the dependent package. So you need a way to cd to it, like

	"preGenerateCommands": [
		"cd $PACKAGE_DIR && the_command"
	]

I honestly think $PACKAGE_DIR was intended to always work like it did, and that dlang/dub-docs#26 was done in error.

[WebFreak001]

this seems not easy to fix without introducing an issue of environment variables mismatching with the direct substitution variables, which was also an issue before.

I think it would be possible to store variables along with the commands, but I'm currently still looking into how well that would fit into the dub architecture.

An alternative would be substituting the variables before execution at build/generate time, but there environment variables on execution are lost.

[veelo]

Understood. Variables are quite flaky. Dub 1.27.0 also has the issue that for a tagetType none package with a sub-package on which it depends, both having preGenerateCommands, the $PACKAGE_DIR in the parent package gets the value of the sub-package. I had to use $ROOT_PACKAGE_DIR instead.

I think the main reason why I need $PACKAGE_DIR is because you never know what the current working directory is inside commands. If Dub would guarantee that the current working dir is set to the location of the dub.json that holds the command, things would just work without silly kludges like cd $PACKAGE_DIR && do stuff.

It is a shame that recipes that needed strange constructions to make them work will break when you try to fix things. Maybe Dub 2.0 is needed?

Thank you for your clean-up efforts!

[veelo]

Since #2217 made it into D 2.100.0, this has turned into a blocker for us, meaning we are stuck at 2.099 :-(

If this issue cannot be resolved, and dlang/dub-docs#26 indeed reflects the intended design, then the corresponding variables must be undefined, so they won't be used with wrong values. And, in order to make preGenerateCommands, preBuildCommands etc. work without these variables, the current working directory for the execution of these commands must be set to the path that contains the Dub .json/.sdl of this package, regardless of its place in the dependency tree or whether it is a sourceLibrary or not.

Can we please get a discussion going of how we are going to move forward?

[thewilsonator]

Can we please get a discussion going of how we are going to move forward?

I would recommend bringing this up at beerconf if it is not fixed soon.

[veelo]

Note that I am not alone in using $PACKAGE_DIR inside preGenerateCommands, here is another example: #2238. In fact I think this is the primary use case for $PACKAGE_DIR (because the current working dir can be anything currently). Ironically, even the Changelog example is broken: https://dlang.org/changelog/2.100.0.html#build-path-variable.

[WebFreak001]

the variables work, just not under sourceLibrary target type.

[veelo]

the variables work, just not under sourceLibrary target type.

Then dlang/dub-docs#26 should be reverted, no?

[WebFreak001]

yes

[rikkimax]

I don't think so.

I've looked at this a bit more thoroughly.

Here is the code that handles PACKAGE_DIR https://github.com/WebFreak001/dub/blob/fa8186cd317e7dbce1e5e7233bd15d2a33cad7f7/source/dub/project.d#L1421

It's in a weird spot.

I think the variable substitution needs its own module. Split into early and late substitutions. Most substitutions should occur where they do so now. PACKAGE_DIR is rare in that it must be done sometime during the analysis but after what dub describe needs.

[WebFreak001]

I have updated code on that and reworked the architecture of that to be more sane, it's just missing the final few pieces to complete, but I can push it already and make a draft PR

[veelo]

I would really appreciate if this regression can be fixed before the next release. It is holding us back from compiler upgrades.

[mdparker]

@WebFreak001 @Geod24 This is the issue Bastiaan brought to us in the January meeting. Any chance on getting this to the finish line soon?

[WebFreak001]

things of concern right now:

• the new $PACKAGE_DIR has been used already / name is ambiguous
• we might want to add $SOURCE_PACKAGE_DIR or something like that

current workarounds:

• change the library type to library instead of sourceLibrary
• @veelo another new workaround I found while trying to get a good solution working here, which doesn't seem to come with any downsides like sourceLibrary: you can use $<uppercase package name>_PACKAGE_DIR to specify from which exact package you want the package path

variable naming rules:

• uppercased name
• - replaced with _

We need to be discussing a better solution here I think. The approach in my rework PR there also does not feel like it's the most scalable one.

Things we need to take into account with the variables:

• when are they evaluated?
• can things like preGenerateCommands / preBuildCommands / external file changes during generation or building affect their values?
  • if yes, do we update the variables in the recipe?
• how do we handle variable deprecation/addition/removal in the future?
• do we support environment variables everywhere or just in commands?
  • should we rather use a special syntax like ${env.foo} so it's possible to know where they come from?

One idea I have right now, which I haven't investigated too much yet, how we could change here is that we introduce a new ${path} syntax and provide a context that can lookup things like

• ${env.ENVIRONMENT_VARIABLE}
• ${target.packagePath}
• ${this.packagePath}

[WebFreak001]

I have been thinking more about the environment variables and think something like the special scoped syntax is something that we could greatly benefit from in the future.

In the meantime, @veelo did using $<uppercase package name>_PACKAGE_DIR solve your blocker?

[veelo]

In the meantime, @veelo did using $<uppercase package name>_PACKAGE_DIR solve your blocker?

Thank you for this suggestion, that might work. I changed it into a dummy (ordinary) library a while ago, to get on: #2251 (comment)

I'll let you know when I find time to try this out.