Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std.path: Fix tilde expansion for HOME=/ #7510

Merged
merged 1 commit into from
Jun 12, 2020
Merged

Conversation

kinke
Copy link
Contributor

@kinke kinke commented May 31, 2020

That's the case for CirrusCI/FreeBSD - the user is root, the USER env var unset and HOME set to /, and the tilde expansion for ~ in that case led to an empty string and according failures in

phobos/std/path.d

Lines 4158 to 4163 in 95e4fb4

immutable tildeUser = "~" ~ environment.get("USER");
immutable path = executeShell("echo " ~ tildeUser).output.strip();
immutable expTildeUser = expandTilde(tildeUser);
assert(expTildeUser == path, expTildeUser);
immutable expTildeUserSlash = expandTilde(tildeUser ~ "/");
assert(expTildeUserSlash == path ~ "/", expTildeUserSlash);

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7510"

@Geod24
Copy link
Member

Geod24 commented May 31, 2020

Looks like a no-brainer. Don't you want it in stable ? Also I'm not sure copying bash's behavior for ~/ is the most sensible, as it will be the only place where // is produced, will it not ?

@kinke
Copy link
Contributor Author

kinke commented May 31, 2020

stable would be somewhat easier for me/LDC, but as that bug has probably been there for ages, I thought it'd be safer to target master. No strong opinion though.

As to 'copying bash's behavior for ~/' - I took care of that first, but then realized that the failing test would then fail again, because it unconditionally expects an added slash compared to ~ alone. While the test can of course be adapted for this (or that whole test only exected if USER is set - although it revealed this bug), I've only later seen that bash doesn't try to get rid of duplicate dir separators at all, unlike Phobos here, so HOME=/ is no special case for bash. Open for suggestions wrt. how to proceed.

@kinke
Copy link
Contributor Author

kinke commented Jun 1, 2020

D:\a\1\dmd\test>"D:\a\1\dmd\dmd2\windows\bin\dmd.exe" -m32mscoff -i run.d || exit /B 6
D:\a\1\dmd\test>run.exe all || exit /B 6
core.exception.AssertError@D:\a\1\dmd\dmd2\windows\bin....\src\phobos\std\algorithm\mutation.d(2790): Swap: lhs internal pointer.

This also happens consistently when running the run testdriver for LDC's AArch64 CI. Definitely needs some investigation.

@thewilsonator thewilsonator merged commit 79cc44b into dlang:master Jun 12, 2020
@kinke kinke deleted the tilde branch June 12, 2020 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants