Fix Issue 11997 - rdmd should search it's binary path for the compiler#250
Fix Issue 11997 - rdmd should search it's binary path for the compiler#250dlang-bot merged 1 commit intodlang:masterfrom joakim-noah:rdmd
Conversation
|
Thanks for your pull request, @joakim-noah! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Some tips to help speed things up:
Bear in mind that large or tricky changes may require multiple rounds of review and revision. Please see CONTRIBUTING.md for more information. Bugzilla references
|
rdmd.d
Outdated
| //writeln("Invoked with: ", args); | ||
| string binPath = thisExePath(); | ||
| if (binPath.exists && binPath.isFile && filenameCmp(baseName(binPath), | ||
| baseName(args[0])) == 0) |
There was a problem hiding this comment.
All these sanity checks necessary? Even if not, may be worth leaving in for now.
There was a problem hiding this comment.
Imho they aren't necessary, just confuse the reader and prevent symlinks, so I would suggest to remove them.
What do you think @CyberShadow?
There was a problem hiding this comment.
I don't think this is the right approach. Regardless, the last check may fail on Windows or other systems with case-insensitive filesystems.
There was a problem hiding this comment.
Not sure what you mean about the approach, but filenameCmp claims to be able to handle case-sensitivity.
There was a problem hiding this comment.
Ah, that's right. Still the matter of the extension.
There was a problem hiding this comment.
I don't know what you mean about the extension, currently compiler does nothing with extensions. It simply passes dmd, ldmd2, or gdmd to spawnProcess.
There was a problem hiding this comment.
C:\Temp\2017-07-12>cat test.d
import std.stdio;
import std.file;
void main(string[] args)
{
writefln!"thisExePath = %s"(thisExePath);
writefln!"args[0] = %s"(args[0]);
}
C:\Temp\2017-07-12>dmd test.d
C:\Temp\2017-07-12>test
thisExePath = C:\Temp\2017-07-12\test.exe
args[0] = test
There was a problem hiding this comment.
Ah, ok, you didn't say what the problem was, so I thought you were referencing something like the wrapper script you mentioned below. I guess this means the last check will fail on Windows, and it'll fall back to the dmd from PATH instead. I'll just remove this third check, doesn't do much even when it works.
wilzbach
left a comment
There was a problem hiding this comment.
Would be nice to get this in before the final 2.075 release of dmd
You would need to rebase onto stable.
This feature makes sense to me in general, but it would be nice to leave a comment about the motivation of tring to find the defaultCompiler executable in the directory of the rdmd executable.
rdmd.d
Outdated
| if (binPath.exists && binPath.isFile && filenameCmp(baseName(binPath), | ||
| baseName(args[0])) == 0) | ||
| { | ||
| string compilerPath = buildPath(dirName(binPath), defaultCompiler); |
There was a problem hiding this comment.
FYI: binPath.dirName.buildPath(defaultCompiler) would be more phobosque ;-)
There was a problem hiding this comment.
I agree, but I prefer the old-fashioned style for this, reads better to me.
CyberShadow
left a comment
There was a problem hiding this comment.
I think this change should take effect only if rdmd is unable to otherwise find a compiler binary - i.e. if --compiler is unspecified or does not contain any path components, AND the compiler binary is not found in PATH.
On Windows, you will need to mind the PathExt environment variable, as users may be expecting rdmd to run a compiler wrapper script they have placed in PATH.
This is also lacking a test.
I don't mess with the one in
I can add one once we figure out the appropriate behavior. |
|
Added the doc comment Seb wanted. |
Ah, I see. I agree, that would be useful, though it could still potentially break users' setups. For example, they might install rdmd with their package manager (probably as part of a dmd/dtools package), then add to their PATH a directory containing a version of The change is still probably worth it, as currently |
|
Whatever we do is going to break some user's setup, but we need to make sure we get the beginners' experience right, because if some noob downloads the compiler tarfile, uncompresses it, and runs They thought For more advanced users who know I'm fine with not rushing to get this into 2.075. It's been broken for years, we can get it right for the next release. |
It's no argument that fixing this would be a good change.
I think that would be excessive (and annoying). I think the current approach proposed here (always picking up the dmd binary from rdmd's executable's directory if there is one) is fine, just needs a test and changelog entry (plus review nits). BTW, what do you think of activating the new behaviour only if there is also a |
I'll get on that later today.
In your scenario above, both the system dmd and the locally-built dmd are most likely in the path. If you prepend your local dmd folder to the path, this PR will change what Seems like a lot of work for an unlikely scenario, better to just add a note to the changelog telling them to use |
How so? Just check for the configuration file when you check for the compiler binary. |
I just told you, each compiler has differently named and placed config files. Why add all that logic for some weirdo intentionally using rdmd with some strange path setup? Tell him to state his intentions clearly, with |
I think we should look into that then before going forward with this. I was thinking mainly about DMD, I don't know how the other compilers deal with this (running them straight out of their tarball), if at all.
That's not a constructive way to look at things. The situation doesn't seem otherworldy to me, and at least in case of DMD, supporting it should be very little effort. |
No, checking all the config files is a dead end.
As I said initially, If you're so worried about not breaking all such niche uses of I think we're more likely to be allowing in broken rather than non-broken uses of |
If you don't want to look into how other compilers behave, then please restrict the new behaviour to
I think that would be fine too. Are you proposing this because it is simpler than checking PATH? FWIW, there is
I am not against changing the behaviour, but I think it would be best if we were to avoid doing so when we can detect with reasonable effort that it would be pointless in the first place, hence the configuration file idea. |
|
This is ridiculous, you have way too much time on your hands. I have updated this PR to get rid of the first three checks. This commit enforces that the adjacent compiler is used, which is what 99% of users expect, and only falls back to the path otherwise. It fixes a long-standing bug in Whether you merge it or throw it away, I don't care at this point. I tried to help the noobs out- I don't use I think this should be rushed into 2.075, but considering this issue sat untouched for 3.5 years now, I wouldn't be surprised if that continued. |
I think we are at the point where it's more worthwhile to research issues thoroughly rather than rushing in fixes, even if it means that contributors' limited time can limit what contributions can be accepted.
Degrading potential users who have an uncommon setup is not only not constructive, but actively prevents having a clear picture of the situation and fallout of the change. So, please refrain from doing that.
Exaggerating your position is not helpful either. The expected way to use binary release archives is to prepend the bin directory to the PATH. Yes, there is an opportunity to improve thing here, but calling things broken is pushing it.
Please get acquainted with our release process. Once the beta period starts, new development continues on the master branch, while the stable branch is used for regression and other urgent fixes. Anything that has an obvious risk of breakage needs to be on the master branch. In any case, if you don't have time to continue working on this, that's OK; I'll have a look at how the other compilers behave and what would best make sense for rdmd to do. |
|
@joakim-noah I had a look at what GDC and LDC do, and it looks like all three compilers support being ran by absolute path from their zip/tarball. The way they achieve this is different between compilers:
So, you're right that checking for configuration files is pointless; I had a wrong assumption that at least the DMD-like drivers (gdmd/ldmd) would have an implementation or equivalent of DMD's sc.ini/dmd.conf. So, I think this change is good as it is; just missing a changelog entry which includes a description of the situation where this change could break a user's setup, and a workaround (e.g. |
|
I've looked at the config file lookup code before: if you scroll up from the block you linked from ldc, it checks for Regarding the changelog, that would require a separate PR in dmd's |
I saw that too, and I think that's fine, because the only way a config file can end up there is if it were placed there by the user, which comes associated with the expectation that compilers will use that config file no matter where they're run from.
Oh, I thought the tools repo had a changelog directory like the others. @wilzbach What do you think, should we add a changelog directory for tools too? |
|
Almost forgot, this also still needs a test... |
|
I will add the test and update. |
It's already checked by the script, so adding a file to FYI: the commit list is parsed as well (like on the other repos), see e.g. https://dlang.org/changelog/2.074.0.html (last entry at the bottom) |
changelog/b11997.dd
Outdated
| first, whereas before, it would default to invoking a compiler | ||
| from the user's path (which it still falls back to if there's | ||
| no D compiler next to rdmd). If you want rdmd to use a specific | ||
| compiler, use the --compiler flag to force it. |
There was a problem hiding this comment.
First time writing a changelog entry, let me know if everything's right.
rdmd_test.d
Outdated
| assert(res.status == -SIGSEGV, format("%s", res)); | ||
| } | ||
|
|
||
| /* https://issues.dlang.org/show_bug.cgi?id=11997- rdmd should search it's |
There was a problem hiding this comment.
Nit, "its" not "it's" (which is always short for "it is")
There was a problem hiding this comment.
I know, :) but you'll have to take that up with Martin, as it's a copy-paste from his bug title. I can still change it here if you prefer.
There was a problem hiding this comment.
Anyone can edit Bugzilla issues, including their titles! Fixed it there now :)
changelog/b11997.dd
Outdated
| @@ -0,0 +1,7 @@ | |||
| Check for a D compiler next to rdmd first | |||
There was a problem hiding this comment.
From the heading list, it won't be clear that this applies to rdmd, so how about:
"rdmd now prefers using a compiler binary from its directory, if one exists"
changelog/b11997.dd
Outdated
| @@ -0,0 +1,7 @@ | |||
| Check for a D compiler next to rdmd first | |||
|
|
|||
| Rdmd will now try to use the D compiler it's installed with | |||
There was a problem hiding this comment.
Backticks around rdmd (plus lowercase) and, --compiler?
changelog/b11997.dd
Outdated
| first, whereas before, it would default to invoking a compiler | ||
| from the user's path (which it still falls back to if there's | ||
| no D compiler next to rdmd). If you want rdmd to use a specific | ||
| compiler, use the --compiler flag to force it. |
There was a problem hiding this comment.
Plus:
To restore the old behaviour of always using the compiler from `PATH`, use `--compiler=dmd`.
changelog/b11997.dd
Outdated
|
|
||
| Rdmd will now try to use the D compiler it's installed with | ||
| first, whereas before, it would default to invoking a compiler | ||
| from the user's path (which it still falls back to if there's |
There was a problem hiding this comment.
"path" should be in uppercase and backticks, so that it's clear the the word refers to the name of an environment variable, and not some nonspecific user path.
| scope(exit) std.file.remove(localDMD); | ||
| res = execute(rdmdApp ~ [modelSwitch, "--force", "--chatty", "--eval=writeln(`Compiler found.`);"]); | ||
| assert(res.status == 1, res.output); | ||
| assert(res.output.canFind(`spawn ["` ~ localDMD ~ `",`)); |
There was a problem hiding this comment.
I think you could do a "positive" test by putting a fake compiler binary in the same directory as rdmd, but it's entirely up to you. Another way would be looking at the output of --dry-run, and simply checking that it would have executed the correct compiler.
|
|
||
| res = execute(rdmdApp ~ [modelSwitch, "--force", "--chatty", "--eval=writeln(`Compiler found.`);"]); | ||
| assert(res.status == 1, res.output); | ||
| assert(res.output.canFind(`spawn ["` ~ localDMD ~ `",`)); |
There was a problem hiding this comment.
Added the test: it would be nice if we didn't expect it to fail, but that would mean copying a whole dmd install over, which would be too much of a PITA. I'm not sure if the --force flag is necessary, didn't need it myself, but all other invocations of --eval in this test file use it, so I stuck it here too.
There was a problem hiding this comment.
I think you could do a "positive" test by putting a fake compiler binary in the same directory as rdmd, but it's entirely up to you.
You mean positive in that rdmd would run and exit successfully? Wouldn't dmd need druntime and phobos and a config file too? Becomes too much of a PITA, especially once you consider other compilers, like ldc.
Another way would be looking at the output of --dry-run, and simply checking that it would have executed the correct compiler.
Yeah, that might work, but you're not even executing the eval=writeln code then, so not much of a difference.
There was a problem hiding this comment.
Wouldn't dmd need druntime and phobos and a config file too? Becomes too much of a PITA, especially once you consider other compilers, like ldc.
Not if the dmd binary in question is just a small stub program that prints something and returns success OSLT.
There was a problem hiding this comment.
I think @CyberShadow means sth. along these lines:
mkdir -p $(ROOT)/dmd_stub
echo 'void main() { import std.stdio; "Hello World".writeln; }' > $(ROOT)/dmd_stub/dmd.d
dmd -of/$(ROOT)/dmd_stub/dmd $(ROOT)/dmd_stub/dmd.dThere was a problem hiding this comment.
Oh, heh, seems pointless. I meant it would be nice if we could actually compile with the adjacent dmd. If we're not going that far, I actually like that it fails, because the first assert will trigger if rdmd pulls a dmd from PATH, because it will unexpectedly compile! :) No need to even check the path in the spawn ["dmd"] invocation then.
There was a problem hiding this comment.
It seems nuts that your tempDir is the one where all your source packages are unpacked, surely there's a better way.
It's not in my hand to change that. That's how the package manager Nix works for building packages. You are only allowed to write inside your build directory because you are not allowed to change anything else on the system this way. The package has only access to that very directory, this is important to prevent any side effects while building and maintain a reproducible build.
Not sure what you mean, these tests simply assume that the dmd compiler is installed somewhere in the system path and that there's no random file/directory named dmd lying around your tempDir, where it runs the tests.
Currently dmd's source directory needs to be placed in the parent directory of the source directory of the tools. I suggest to expect the dmd source inside the tools directory. This is a much cleaner way because you don't know your environment and ../dmd might be an old dmd needed by the user or whatever which might lead to strange errors.
This would also fix my issue with Nix and prevent my current workaround.
There was a problem hiding this comment.
Currently dmd's source directory needs to be placed in the parent directory of the source directory of the tools. I suggest to expect the dmd source inside the tools directory.
Not sure how that's related to this discussion, but potentially having a copy of every component inside the directory of every component doesn't sound better than each component expecting its siblings to be in its parent directory.
@ThomasMader Feel free to submit a PR which changes the rdmdApp and localDMD paths to a more unique subdirectory of tempDir().
There was a problem hiding this comment.
Which means that dtools is in /tmp/nix-build-dtools-2.078.0.drv-5/dtools and I also need to place the dmd source at /tmp/nix-build-dtools-2.078.0.drv-5/dmd which is the same path localDMD wants to be written to. :-)
I don't see how you reached that conclusion. I don't see how the location of the dmd source code is in any way relevant here.
The problem here is that both rdmd_test and Nix do not do a sufficiently good job at namespacing their temporary files in $TMP. The collision could be resolved from etiher side, and you seem to have chosen the D side, but I would suggest to also raise an issue in Nix's issue tracker.
There was a problem hiding this comment.
Oh, I think I understand what you meant now regarding the DMD dir:
Line 13 in 53735e9
It's cloned automatically by the Makefile. I don't know much about Nix packaging, but if you care about the package being buildable into the future or reproducible builds, you will probably want to provide the contents of the dmd repository separately, and perhaps specify the path to it using the DMD_DIR makefile parameter.
There was a problem hiding this comment.
I don't know much about Nix packaging, but if you care about the package being buildable into the future or reproducible builds, you will probably want to provide the contents of the dmd repository separately, and perhaps specify the path to it using the DMD_DIR makefile parameter.
While I was preparing #287 I realized that I already can set DMD_DIR and move it inside the tools directory myself.
So it's no real issue for me anymore.
The remaining question is if this should be the default behavior but should probably better be discussed in the PR.
|
Updated to fix your nits and describe exactly the situation that motivated this change. |
changelog/b11997.dd
Outdated
| @@ -0,0 +1,14 @@ | |||
| rdmd now checks for a D compiler binary in the directory | |||
| it's in first | |||
There was a problem hiding this comment.
First changelog line has to be all on its own line (not wrapped)... CI / changelog generation will fail otherwise.
|
@wilzbach The changelog entry doesn't seem to be picked up, any idea why off the top of your head? |
|
Oh, I'm guessing dlang/dlang.org#1826 was the cause. Well, LGTM, so merging. |
|
Thanks, looking forward to trying |
Yep, Martin reverted the generation of the _pending file and _pre exists and won't be regenerated... |
@joakim-noah btw -> https://dlang.org/changelog/2.076.0_pre.html#b11997 |
|
Well, for the matter of public record, noting here that this broke the DMD setup on my laptop (as I don't build rdmd there), so that certainly wasn't a theoretical concern. The |
rdmdshould work out of the box now, without having to add it to yourPATH, tested with dmd and ldc, which just started shipping withrdmd. Would be nice to get this in before the final 2.075 release of dmd, pinging @MartinNowak.Only drawback I can see is that if someone takes rdmd and puts it in some arbitrary directory with a file that's happened to be named
dmdorldmd2,rdmdwill now try to run it. Seems far-fetched, but the real solution is to just integraterdmdwith dmd and ldc, so this external script doesn't always have to go find them.