Fix #895, so that dub looks for a compiler next to itself first #1208

joakim-noah · 2017-07-27T11:42:17Z

Since I didn't get a response about how checking for a compiler next to dub should interact with the one specified in a DUB config file, I went ahead and implemented what I thought makes sense. I can add some tests once the behavior is approved.

Also puts some restrictions on the defaultCompiler specified in a DUB settings.json, such that it must be a compiler name or an absolute path, including a tilde-prefixed local path. Relative paths are no longer
allowed, as they make no sense for how dub invokes the compiler. You could argue that even a compiler name should be disallowed, as it's unlikely anyone will specify one that's not already on the default list that dub tries, but I suppose the sdc devs may want to try out dub some day. ;)

If a compiler name like sdc is specified in a DUB settings.json, I have dub check for other common compilers in the PATH only if sdc isn't found next to dub or on the PATH (Update: which is probably why the test for #895 now fails on Travis CI, though the test will be changed), though you could argue the user only wants sdc used. However, dub always says what compiler it's using and if the user hasn't provided an sdc, it's a convenience to search for another compiler. I could change it to only check the PATH for sdc in that case, if wanted, as I do when checking next to dub.

Finally, all this changes nothing about specifying a compiler with --compiler, where none of this is used and you can specify any D compiler you want, including one with a relative path.

dlang-bot · 2017-07-27T11:42:18Z

Thanks for your pull request, @joakim-noah!

MartinNowak · 2017-08-03T21:57:22Z

Why not interpret relative paths in settings.json as relative to dir containing the dub executable?

joakim-noah · 2017-08-03T22:31:35Z

Possible, but then inconsistent with --compiler, which I believe will invoke relative to getcwd. Could still do that, as we assume someone writing a config file is sane, ;) just as long as we document the difference.

MartinNowak · 2017-08-04T06:49:31Z

Possible, but then inconsistent with --compiler, which I believe will invoke relative to getcwd. Could still do that, as we assume someone writing a config file is sane, ;) just as long as we document the difference.

I'd go for it, as it allows more flexibility for packaging (e.g. different bin folders).
We could use some $DUB_BIN_PATH variable instead of relative paths to make it more explicit.
Can the settings.json be located next to dub or somehow be part of the packaging?

joakim-noah · 2017-08-04T23:31:04Z

We could use some $DUB_BIN_PATH variable instead of relative paths to make it more explicit.

Good idea, will add that.

Can the settings.json be located next to dub or somehow be part of the packaging?

dub looks for it first in the system (/var/lib/dub on Posix, scroll up a bit to see), then relative to the binary at ../etc/dub/settings.json, and finally in ~/.dub. ldc uses the second to change the default compiler name to ldc, but that's it.

joakim-noah · 2017-08-10T22:15:36Z

Alright, modified this pull to check for %%dubbinarypath%% also, similar to the way ldc does it. I'll fix and write more tests once this behavior is approved.

joakim-noah · 2017-08-15T01:54:28Z

@s-ludwig, let me know if this pull is okay, would be good to get this in before the next dmd/ldc releases.

s-ludwig · 2017-08-29T22:23:26Z

Sorry for the late reply, this slipped past me. Two notes:

DUB_BIN_PATH or DUB_BINARY_PATH would be more consistent with the other variables that DUB defines
Is the changed lookup logic still needed with the new possibility to specify an executable-relative path in settings.json?

joakim-noah · 2017-08-30T15:21:34Z

DUB_BIN_PATH or DUB_BINARY_PATH would be more consistent with the other variables that DUB defines

Those are all environment variables normally though, right? I thought this should be different because it's specific to the config file. Doesn't really matter to me though, just let me know what you want.

Is the changed lookup logic still needed with the new possibility to specify an executable-relative path in settings.json?

The only changed lookup logic is to first search for a compiler name specified in the config file in the PATH. The newly added lookup in the directory next to dub could just be replaced by specifying an executable-relative path in the config file instead, as you say, but it's a convenience to not have to provide a config file at all, ie it always looks next to dub.

s-ludwig · 2017-08-31T12:44:15Z

Those are all environment variables normally though, right? I thought this should be different because it's specific to the config file. Doesn't really matter to me though, just let me know what you want.

My idea was that we could support real environment variables, too, at some point, and this special case could then just naturally merge with that.

The newly added lookup in the directory next to dub could just be replaced by specifying an executable-relative path in the config file instead, as you say, but it's a convenience to not have to provide a config file at all, ie it always looks next to dub.

Okay, it's mostly just a fuzzy concern on my side right now, because I haven't really thought through the possible implications/issues. One thing that gets a bit hairy is when one explicitly wants to select the compiler in PATH instead of the one in the local directory, but maybe that's a bit superficial. Generally I'd just try to avoid changes that serve no concrete need (i.e. if a config file in the compiler distribution already works for the initial use case).

joakim-noah · 2017-08-31T13:05:45Z

My idea was that we could support real environment variables, too, at some point, and this special case could then just naturally merge with that.

OK, will change it.

One thing that gets a bit hairy is when one explicitly wants to select the compiler in PATH instead of the one in the local directory, but maybe that's a bit superficial.

Just specify --compiler=gdc then, as noted for rdmd with its equivalent pull. You're right that you couldn't do that from the config file anymore though, if dub had the same compiler next to it.

Generally I'd just try to avoid changes that serve no concrete need (i.e. if a config file in the compiler distribution already works for the initial use case).

It is true that adding that option to use a compiler relative to dub's path to the config file, which wasn't there before this pull, is potentially redundant with checking for a compiler next to dub automatically, but only if every compiler that ships dub adds a config file that explicitly says where dub is relative to the compiler.

Since most will stick dub right next to the compiler, this saves them that trouble. It's sufficiently common that I think it's worth having both.

s-ludwig · 2017-09-05T07:57:30Z

Having rdmd now behave like this is a good argument. It makes sense to stay consistent within the tools ecosystem.

joakim-noah · 2017-09-20T09:46:02Z

source/dub/dub.d

+		if (m_defaultCompiler.length && m_defaultCompiler.isAbsolute)
+			return;
+
+		auto dubPrefix = m_defaultCompiler.matchFirst(ctRegex!(`^DUB_BINARY_PATH`));


Switched to DUB_BINARY_PATH like you asked.

joakim-noah · 2017-09-20T09:55:05Z

test/issue895-local-configuration.sh

@@ -14,9 +15,11 @@ if [ -e ~/.dub/settings.json ]; then
 	die $LINENO 'Found existing user wide DUB configuration. Aborting.'
 fi

-if ! { ${DUB} describe --single issue103-single-file-package.d 2>&1 || true; } | grep -cF 'Unknown compiler: foo'; then
+if ! { ${DUB} describe --single issue103-single-file-package.d 2>&1 || true; } | grep -cF 'Unknown compiler: '; then


This test should now pass, but removed the compiler name foo because my new code passes the full path of a compiler next to dub to the settings and this error output. I'm adding more tests for the new behavior and will update the patch soon.

joakim-noah · 2017-10-13T10:05:32Z

Finally spent an hour and updated this pull to test all new code paths, should be ready to merge.

joakim-noah · 2017-10-23T10:22:17Z

Ping, @s-ludwig or @MartinNowak, PR passes most CI, except for some scenarios because of the upstream druntime headers, which are broken unrelated to this pull.

joakim-noah · 2017-12-04T04:13:15Z

Ping, would be good to get this in for the next release, so dub works out of the box with dmd.

s-ludwig · 2017-12-04T09:29:45Z

The only issue I can see is that the "$" is missing before "DUB_BINARY_PATH". Otherwise it looks good to merge, the Windows failure is unrelated (we should really clean this up at some point, so that AppVeyor can also be marked as a required check).

joakim-noah · 2017-12-04T09:44:26Z

OK, do you want me to add the $ prefix for the path setting? I just followed what you wrote above.

s-ludwig · 2017-12-04T10:01:48Z

Yeah, sorry for that, I was indeed writing ambiguously there. So in the path, it should be written as "$DUB_BINARY_PATH/../foo", so that it looks like an sh style environment variable basically.

joakim-noah · 2017-12-04T10:12:39Z

Will do, in the meantime, I rebased to get the CI to run again.

Update: Added the $ prefix.

put some restrictions on the defaultCompiler specified in a DUB settings.json, such that it must be a compiler name, a path relative to dub, or an absolute path, including a tilde-prefixed local path. Unqualified relative paths are no longer allowed from the settings file, as they make no sense for how dub invokes the compiler.

joakim-noah · 2017-12-04T18:23:13Z

Is codecov broken, @wilzbach? It says I only cover 45% of my patch with the tests, when I tried to hit all of it.

s-ludwig added the needs input label Sep 6, 2017

joakim-noah added work in progress and removed needs input labels Sep 7, 2017

joakim-noah commented Sep 20, 2017

View reviewed changes

joakim-noah added enhancement needs review and removed work in progress labels Oct 13, 2017

s-ludwig removed enhancement needs review labels Dec 4, 2017

joakim-noah merged commit bf095d8 into dlang:master Dec 6, 2017

joakim-noah deleted the next branch December 13, 2017 06:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #895, so that dub looks for a compiler next to itself first #1208

Fix #895, so that dub looks for a compiler next to itself first #1208

joakim-noah commented Jul 27, 2017 •

edited

Loading

dlang-bot commented Jul 27, 2017 •

edited

Loading

MartinNowak commented Aug 3, 2017

joakim-noah commented Aug 3, 2017

MartinNowak commented Aug 4, 2017

joakim-noah commented Aug 4, 2017

joakim-noah commented Aug 10, 2017

joakim-noah commented Aug 15, 2017

s-ludwig commented Aug 29, 2017

joakim-noah commented Aug 30, 2017

s-ludwig commented Aug 31, 2017

joakim-noah commented Aug 31, 2017 •

edited

Loading

s-ludwig commented Sep 5, 2017

joakim-noah Sep 20, 2017 •

edited

Loading

joakim-noah Sep 20, 2017

joakim-noah commented Oct 13, 2017

joakim-noah commented Oct 23, 2017

joakim-noah commented Dec 4, 2017

s-ludwig commented Dec 4, 2017

joakim-noah commented Dec 4, 2017

s-ludwig commented Dec 4, 2017

joakim-noah commented Dec 4, 2017 •

edited

Loading

joakim-noah commented Dec 4, 2017

Fix #895, so that dub looks for a compiler next to itself first #1208

Fix #895, so that dub looks for a compiler next to itself first #1208

Conversation

joakim-noah commented Jul 27, 2017 • edited Loading

dlang-bot commented Jul 27, 2017 • edited Loading

MartinNowak commented Aug 3, 2017

joakim-noah commented Aug 3, 2017

MartinNowak commented Aug 4, 2017

joakim-noah commented Aug 4, 2017

joakim-noah commented Aug 10, 2017

joakim-noah commented Aug 15, 2017

s-ludwig commented Aug 29, 2017

joakim-noah commented Aug 30, 2017

s-ludwig commented Aug 31, 2017

joakim-noah commented Aug 31, 2017 • edited Loading

s-ludwig commented Sep 5, 2017

joakim-noah Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

joakim-noah Sep 20, 2017

Choose a reason for hiding this comment

joakim-noah commented Oct 13, 2017

joakim-noah commented Oct 23, 2017

joakim-noah commented Dec 4, 2017

s-ludwig commented Dec 4, 2017

joakim-noah commented Dec 4, 2017

s-ludwig commented Dec 4, 2017

joakim-noah commented Dec 4, 2017 • edited Loading

joakim-noah commented Dec 4, 2017

joakim-noah commented Jul 27, 2017 •

edited

Loading

dlang-bot commented Jul 27, 2017 •

edited

Loading

joakim-noah commented Aug 31, 2017 •

edited

Loading

joakim-noah Sep 20, 2017 •

edited

Loading

joakim-noah commented Dec 4, 2017 •

edited

Loading