env var replacement not supporting `${VAR_NAME}` syntax #1392

MartinNowak · 2018-02-22T15:16:44Z

This is commonly needed to join pathes or a suffix, e.g. $PACKAGE_DIRmylib.a (as unfortunately PACKAGE_DIR ends on /).
Expanding variables is straighforward, https://github.com/MartinNowak/alertd/blob/cbc4459bbdbcc8c799413f58a591f6d28205f147/src/alertd.d#L503.

- simplified and cleaner env var replacement using straighforward regex replaceAll - support ${MY_VAR}suffix replacements

dlang-bot · 2018-02-22T15:16:46Z

Thanks for your pull request, @MartinNowak!

s-ludwig · 2018-02-22T15:25:27Z

Did you check if we get any significant compile-time regressions due to ctRegex?

Otherwise LGTM.

BTW, I'll go through the commit history to add missing change log entries once more, but I already wanted to suggest that we switch to a mode where each significant PR must add a change log entry, because I simply didn't manage to manually author them in time recently. We should probably add a CONTRIBUTING file at some point mentioning this.

MartinNowak · 2018-02-22T15:50:48Z

Did you check if we get any significant compile-time regressions due to ctRegex?

Yeah, replaced it with a runtime regex (btw, nowadays even enum re = regex("") is slow, as it merely does the same as ctRegex (dlang/phobos#5722)). Regexes are cached by std.regex, so it shouldn't be a runtime problem.

s-ludwig · 2018-02-22T16:41:11Z

Regexes are cached by std.regex, so it shouldn't be a runtime problem.

Didn't know that. Then we can also remove the manual caching that is done in other places within DUB.

MartinNowak · 2018-02-22T17:02:15Z

Of course it's messier than explainable, seems like NativePath only preserves trailing separators on posix, so one would need ${PACKAGE_PATH}foobar on posix but ${PACKAGE_PATH}\\foobar for Windows. Given that concatenating suffixes is currently not possible the risk of breaking sth. by removing the slash from PACKAGE_PATH on Posix doesn't seem to big, so I'll go for that to not create an even worse mess.

BTW, I think it would be a net positive for dub to get rid of NativePath.

Slash behavior originally introduced here vibe-d/vibe.d@970a022#diff-3620332feace439ec637b023bc2e7a01R262, and apparently broken here vibe-d/vibe.d@344fe34#diff-47d6be40e29a186df00d5f3bfad92671R36.

MartinNowak · 2018-02-22T20:21:57Z

Didn't know that. Then we can also remove the manual caching that is done in other places within DUB.

Depends on expectations, https://github.com/dlang/phobos/blob/b26aad3209c1adb3006ff7003c44a7f0cdde06d9/std/regex/package.d#L364, 8 entries isn't necessarily enough, but processVars calls are coming all at once.

MartinNowak · 2018-02-23T21:03:18Z

The slash was introduced with 4d4017c to fix #268. It also makes sense for output such as dub list to append a slash, so I'd suggest to just remove the slash in env var expansion.

MartinNowak · 2018-02-23T21:25:10Z

Does that look reasonable? It seems that the chances for breakage are small as $PATHsuffix wasn't possible beforehand, so mostly cd $PATH && ./script were used.
Could subtly break weird path based comparisons or concatenations in scripts though.

wilzbach · 2018-03-23T20:14:55Z

but I already wanted to suggest that we switch to a mode where each significant PR must add a change log entry, because I simply didn't manage to manually author them in time recently. We should probably add a CONTRIBUTING file at some point mentioning this.

If you add a changelog entry to changelog like DMD does, the files will be displayed at https://dlang.org/changelog/pending.html and automatically part of the release announcement.

I recently added support for this on tools/dlang.org side for this:
dlang/tools#302
dlang/dlang.org#2163

Integrating the GitHub issues (like @dlang-bot currently does) is more work and I wasn't sure whether that's wanted, so I left that one out for now.

wilzbach · 2018-03-23T20:17:14Z

Regexes are cached by std.regex, so it shouldn't be a runtime problem.

With dlang/phobos#6268 (in the upcoming 2.080) std.regex caching got a big improvement.
So there's some reason to use static varRE, but OTOH it's probably not worth the troubles.

wilzbach

Does that look reasonable? It seems that the chances for breakage are small as $PATHsuffix wasn't possible beforehand, so mostly cd $PATH && ./script were used.
Could subtly break weird path based comparisons or concatenations in scripts though.

It doesn't break anything in the ~40 projects on Jenkins, so that's a good start.
And yes not using a trailing looks and thus having a unified behavior for the path variables looks very reasonable to me.

wilzbach · 2018-03-23T20:23:09Z

source/dub/project.d

+	enum isPath = true;
+
+	version (Posix) enum sep = "/";
+	else version (Windows) enum sep = `\`;


aka as std.path.dirSeparator

wilzbach · 2018-03-23T20:27:29Z

source/dub/project.d

+	if (name == "PACKAGE_DIR")
+		path = pack.path;
+	else if (name == "ROOT_PACKAGE_DIR")
+		path = project.rootPackage.path;

 	if (name.endsWith("_PACKAGE_DIR")) {
 		auto pname = name[0 .. $-12];
 		foreach (prj; project.getTopologicalPackageList())
 			if (prj.name.toUpper().replace("-", "_") == pname)


FWIW there's no need to allocate for this check,sth. like this should work fine and is @nogc:
prj.name.asUpperCase.map!(a == '-' ? '_' : a).equal(pname)

- simplified and cleaner env var replacement using straighforward regex replaceAll - support ${MY_VAR}suffix replacements

wilzbach · 2018-07-11T03:21:46Z

Addressed my review myself and rebased (the changelog entry is now in changelog/).

- simplified and cleaner env var replacement using straighforward regex replaceAll - support ${MY_VAR}suffix replacements

wilzbach · 2018-07-21T17:27:46Z

Rebased again :/

- simplified and cleaner env var replacement using straighforward regex replaceAll - support ${MY_VAR}suffix replacements

- cached by std.regex

- expand $VARS without trailing slash/backslash (for concatenation e.g. `${PACKAGE_PATH}/subpath`)

This was broken by the refactoring in dlang#1392 (commit 14c00c4)

MartinNowak added a commit to MartinNowak/dub that referenced this pull request Feb 22, 2018

fix dlang#1392 - env var replacement not supporting ${VAR_NAME} syntax

424c786

- simplified and cleaner env var replacement using straighforward regex replaceAll - support ${MY_VAR}suffix replacements

s-ludwig added the auto-merge label Feb 22, 2018

dlang-bot removed the auto-merge label Feb 23, 2018

MartinNowak force-pushed the env_vars branch 2 times, most recently from a8f5adb to 73ec880 Compare February 23, 2018 22:48

wilzbach approved these changes Mar 23, 2018

View reviewed changes

wilzbach added this to the 1.9.0 milestone Mar 23, 2018

wilzbach mentioned this pull request May 2, 2018

Build settings vars #1450

Merged

wilzbach force-pushed the env_vars branch from 73ec880 to 7f3388c Compare July 11, 2018 03:17

wilzbach pushed a commit to MartinNowak/dub that referenced this pull request Jul 11, 2018

fix dlang#1392 - env var replacement not supporting ${VAR_NAME} syntax

87a40bd

- simplified and cleaner env var replacement using straighforward regex replaceAll - support ${MY_VAR}suffix replacements

wilzbach removed this from the 1.9.0 milestone Jul 11, 2018

wilzbach added the auto-merge label Jul 11, 2018

wilzbach added this to the 1.11 milestone Jul 21, 2018

wilzbach force-pushed the env_vars branch from e50ebd2 to 2a5ba31 Compare July 21, 2018 17:11

wilzbach pushed a commit to MartinNowak/dub that referenced this pull request Jul 21, 2018

fix dlang#1392 - env var replacement not supporting ${VAR_NAME} syntax

f010bbd

- simplified and cleaner env var replacement using straighforward regex replaceAll - support ${MY_VAR}suffix replacements

dlang-bot removed the auto-merge label Jul 21, 2018

MartinNowak added 2 commits July 27, 2018 14:28

fix dlang#1392 - env var replacement not supporting ${VAR_NAME} syntax

14c00c4

- simplified and cleaner env var replacement using straighforward regex replaceAll - support ${MY_VAR}suffix replacements

use runtime regex to avoid compile-time slowdown

1fdf47e

- cached by std.regex

MartinNowak and others added 3 commits July 27, 2018 14:29

cleanup trailing slash/backslash behavior of expanded vars

afcbcf3

- expand $VARS without trailing slash/backslash (for concatenation e.g. `${PACKAGE_PATH}/subpath`)

Address review

7a11e4f

Fix Path deprecation

91ebbd9

MartinNowak force-pushed the env_vars branch from 6ad7a78 to 91ebbd9 Compare July 27, 2018 12:30

MartinNowak added the auto-merge label Jul 27, 2018

MartinNowak merged commit f447add into dlang:master Jul 27, 2018

MartinNowak deleted the env_vars branch July 27, 2018 19:32

AntonMeep mentioned this pull request Sep 4, 2018

Regression: Dub v1.11.0 single mode is broken #1551

Closed

Geod24 added a commit to Geod24/dub that referenced this pull request Sep 4, 2018

Fix 1551: Escaping a variable ($$) is ignored

dad0b91

This was broken by the refactoring in dlang#1392 (commit 14c00c4)

Geod24 added a commit to Geod24/dub that referenced this pull request Sep 5, 2018

Fix 1551: Escaping a variable ($$) is ignored

65e9b8c

This was broken by the refactoring in dlang#1392 (commit 14c00c4)

Geod24 added a commit to Geod24/dub that referenced this pull request Sep 5, 2018

Fix 1551: Escaping a variable ($$) is ignored

b32629f

This was broken by the refactoring in dlang#1392 (commit 14c00c4)

Geod24 added a commit to Geod24/dub that referenced this pull request Sep 8, 2018

Fix 1551: Escaping a variable ($$) is ignored

1271345

This was broken by the refactoring in dlang#1392 (commit 14c00c4)

wilzbach mentioned this pull request Feb 28, 2019

dub.project: Improve variable expansion logic #1641

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

env var replacement not supporting `${VAR_NAME}` syntax #1392

env var replacement not supporting `${VAR_NAME}` syntax #1392

MartinNowak commented Feb 22, 2018

dlang-bot commented Feb 22, 2018

s-ludwig commented Feb 22, 2018

MartinNowak commented Feb 22, 2018 •

edited

Loading

s-ludwig commented Feb 22, 2018 •

edited

Loading

MartinNowak commented Feb 22, 2018 •

edited

Loading

MartinNowak commented Feb 22, 2018 •

edited

Loading

MartinNowak commented Feb 23, 2018

MartinNowak commented Feb 23, 2018 •

edited

Loading

wilzbach commented Mar 23, 2018

wilzbach commented Mar 23, 2018 •

edited

Loading

wilzbach left a comment

wilzbach Mar 23, 2018

wilzbach Mar 23, 2018

wilzbach commented Jul 11, 2018 •

edited

Loading

wilzbach commented Jul 21, 2018

env var replacement not supporting ${VAR_NAME} syntax #1392

env var replacement not supporting ${VAR_NAME} syntax #1392

Conversation

MartinNowak commented Feb 22, 2018

dlang-bot commented Feb 22, 2018

s-ludwig commented Feb 22, 2018

MartinNowak commented Feb 22, 2018 • edited Loading

s-ludwig commented Feb 22, 2018 • edited Loading

MartinNowak commented Feb 22, 2018 • edited Loading

MartinNowak commented Feb 22, 2018 • edited Loading

MartinNowak commented Feb 23, 2018

MartinNowak commented Feb 23, 2018 • edited Loading

wilzbach commented Mar 23, 2018

wilzbach commented Mar 23, 2018 • edited Loading

wilzbach left a comment

Choose a reason for hiding this comment

wilzbach Mar 23, 2018

Choose a reason for hiding this comment

wilzbach Mar 23, 2018

Choose a reason for hiding this comment

wilzbach commented Jul 11, 2018 • edited Loading

wilzbach commented Jul 21, 2018

env var replacement not supporting `${VAR_NAME}` syntax #1392

env var replacement not supporting `${VAR_NAME}` syntax #1392

MartinNowak commented Feb 22, 2018 •

edited

Loading

s-ludwig commented Feb 22, 2018 •

edited

Loading

MartinNowak commented Feb 22, 2018 •

edited

Loading

MartinNowak commented Feb 22, 2018 •

edited

Loading

MartinNowak commented Feb 23, 2018 •

edited

Loading

wilzbach commented Mar 23, 2018 •

edited

Loading

wilzbach commented Jul 11, 2018 •

edited

Loading