Skip to content

Apply auto-tester's patching to upstream and avoid the need for it#8137

Closed
wilzbach wants to merge 1 commit intodlang:masterfrom
wilzbach:auto-tester
Closed

Apply auto-tester's patching to upstream and avoid the need for it#8137
wilzbach wants to merge 1 commit intodlang:masterfrom
wilzbach:auto-tester

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Apr 5, 2018

Applies https://github.com/braddr/at-client/blob/master/src/diff-dmd-win64.diff to master.
and thus ideally this removes the need for the auto-tester to do its patching.

I wasn't sure what to do about via patching disabled runnable/test15779.d test as it passes on AppVeyor. Is this still an issue?

@braddr - I guess at the moment this will fail and that's why braddr/at-client#8 would be useful, s.t. (1) this can be tested before and (2) master and stable don't need to be upgraded in lockstep.
However, I guess simply disabling the patching would work fine too. stable isn't particularily busy and I can back-port these changes to stable in a quick follow-up PR.
If you want we can also do all three repos in one stroke (making similar PRs for druntime + phobos's patching shouldn't be too hard once it's disabled).


@ other reviewers: this PR depends on Brad and changes in the auto-tester infrastructure. Please do not merge without his feedback and approval.

edit: don't get confused by "Pass: 8" on the auto-tester - it didn't run the tests on Windows:

image

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

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 fetch digger
dub run digger -- build "master + dmd#8137"

auto-tester-build:
cd src
$(MAKE) -f win32.mak auto-tester-build
make -f win32.mak auto-tester-build
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this change?

{
case "win32": envData.ccompiler = "dmc"; break;
case "win64": envData.ccompiler = `\"Program Files (x86)"\"Microsoft Visual Studio 10.0"\VC\bin\amd64\cl.exe`; break;
case "win64": envData.ccompiler = environment.get("VCBIN_DIR") ~ `\cl.exe`; break;
Copy link
Contributor

@marler8997 marler8997 Apr 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this done as a patch instead of pushing directly to the repo?

Anyway, we might want the logic to be a bit more "forgiving" and helpful in "bad environment" cases. Maybe something like this:

auto vcbinDir = environment.get("VCBIN_DIR",
    `"C:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\bin\amd64"`);
if (!isDir(vcbinDir))
{
    writefln("Error: the Visual Studio C compiler bin directory does not exist (%s)", vcbinDir);
    return 1; // fail
}
envData.ccompiler = vcbinDir ~ `\cl.exe`;

NOTE: I added "C:" to the default vcbinDir path because windows only installs Visual Studio to the C drive and without this you break 64-bit testing if dmd is on another drive. I actually have dmd on another drive but I've never run into this problem since I've never gotten the dmd test suite to work on windows :)

@rainers
Copy link
Member

rainers commented Apr 7, 2018

I wasn't sure what to do about via patching disabled runnable/test15779.d test as it passes on AppVeyor. Is this still an issue?

should be fixed by dlang/druntime#2129, see https://issues.dlang.org/show_bug.cgi?id=18547

@braddr
Copy link
Member

braddr commented Apr 7, 2018

Essentially all of this pull request is obsolete at this point.

The only file still being patched locally to the auto-tester in the dmd package is win32.mak, and I really hate those changes. Dancing between versions and implementations of make is... horrible. I'm very open to suggestions on how to make that suck less.

@JinShil
Copy link
Contributor

JinShil commented Apr 7, 2018

Dancing between versions and implementations of make is... horrible. I'm very open to suggestions on how to make that suck less.

IMO, the way to make the whole make system cross platform, portable, and easily maintainable by all is to incrementally convert it all to D.

@wilzbach
Copy link
Contributor Author

wilzbach commented Apr 9, 2018

Essentially all of this pull request is obsolete at this point.

Cool! Thanks a lot!

@wilzbach wilzbach closed this Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants