Add ability to override DFLAGS env variable in test suite by JinShil · Pull Request #7845 · dlang/dmd

JinShil · 2018-02-06T10:49:53Z

The test suite makefile adds the import directories for druntime and phobos to the DFLAGS environment variable:

Lines 173 to 177 in c55c9be

    
           DRUNTIME_PATH=../../druntime 
        
           PHOBOS_PATH=../../phobos 
        
           # link against shared libraries (defaults to true on supported platforms, can be overridden w/ make SHARED=0) 
        
           SHARED=$(if $(findstring $(OS),linux freebsd),1,) 
        
           DFLAGS=-I$(DRUNTIME_PATH)/import -I$(PHOBOS_PATH) -L-L$(PHOBOS_PATH)/generated/$(OS)/$(BUILD)/$(MODEL)

This makes it impossible to write tests for no- or minimal-runtime programming, due to the implicit import of object.d. See #7825 for evidence.

This PR adds a DFLAGS: test variable that clears the DFLAGS environment variable enabling us to write no- or minimal-runtime tests among other potential possibilities.

dlang-bot · 2018-02-06T10:49:54Z

Thanks for your pull request, @JinShil!

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.

wilzbach · 2018-02-06T11:16:26Z

The CI failures are correct but unrelated to this PR - see #7838 (comment)

wilzbach · 2018-02-06T11:23:21Z

DFLAGS=-I$(DRUNTIME_PATH)/import -I$(PHOBOS_PATH) -L-L$(PHOBOS_PATH)/generated/$(OS)/$(BUILD)/$(MODEL)

Interesting. Shouldn't it overwrite the default dmd.conf with -conf= too? I guess it didn't make a difference to far because the auto-generated one is more or less identical to this command.
(edit: I submitted #7846 - let's see what breaks)

I fear a bit that exposing DFLAGS could be abused or confused with REQUIRED_ARGS or PERMUTE_ARGS. How about a NO_DFLAGS setting that - if set - would set the DFLAGS to -conf=? Then REQUIRED_ARGS / PERMUTE_ARGS / ARG_SETS could be used as usual.

JinShil · 2018-02-06T11:30:50Z

How about a NO_DFLAGS setting that - if set - would set the DFLAGS to -conf=?

-conf= is already being set:

dmd/test/d_do_test.d

Line 677 in c55c9be

    
           string command = format("%s -conf= -m%s -I%s %s %s -od%s -c %s %s", envData.dmd, envData.model, input_dir,

A NO_DFLAGS: test variable that simply wipes out the DFLAGS environment variable would also work for my specific needs, but I thought the DFLAGS: test variable might open up few more possibilities. I'm impartial; either way will work for me.

wilzbach · 2018-02-06T11:42:56Z

I'm impartial; either way will work for me.

Same here. It was just an idea. Has someone a (strong) opinion about this?

JinShil · 2018-02-08T13:47:45Z

After giving this some thought, I think the NO_DFLAGS: test variable would probably be the better solution to, as was alluded to, ensure we have consistent and predictable usage of our test variables. NO_DFLAGS: has been implemented.

wilzbach · 2018-02-08T13:51:56Z

test/d_do_test.d


+    // Clear the DFLAGS environment variable if it was specified in the test file
+    if (testArgs.noDflags)
+        environment.remove("DFLAGS");


edit: ah I see `-conf=` is already set at other parts here.

wilzbach

Looks reasonable to me.

WalterBright · 2018-02-08T17:15:47Z

Shouldn't

DFLAGS=

do the trick of setting it to nothing?

JinShil · 2018-02-08T22:25:11Z

Shouldn't DFLAGS= do the trick of setting it to nothing?

Where? In Makefile? If you set DFLAGS to nothing in Makefile then it applies to all tests. The goal with this PR is to set it to nothing for only specific tests. To do that it must be done in the testxxx.d file, and in there DFLAGS= syntax is not supported.

Originally, I created a DFLAGS: test variable that would override the DFLAGS environment variable, but then that leaves us with too many different ways to set compiler flags: DFLAGS, REQUIRED_ARGS, PERMUTE_ARGS, and ARGS_SETS. That will only lead to inconsistency and unpredictability in how the compiler flags are set for the test suite. The NO_DFLAGS test argument allows us to clear the DFLAGS environment variable individually for each test, without opening the flood gates to all the different creative ways one might choose to set compiler flags.

It could be argued that we could simplify the test suite by removing all the other test variables and only allowing DFLAGS to be overridden or appended, but that would require changing every test file in the repository, and you already scolded me twice for pulling PRs that touch too many files.

WalterBright · 2018-02-09T00:42:42Z

The goal with this PR is to set it to nothing for only specific tests. To do that it must be done in the testxxx.d file, and in there DFLAGS= syntax is not supported.

Sorry, the DFLAGS: syntax. Since these are specific to each test file, there is no earthly reason to have:

DFLAGS:

in there unless one wants to clear DFLAGS.

The NO_DFLAGS test argument allows us to clear the DFLAGS environment variable individually for each test, without opening the flood gates to all the different creative ways one might choose to set compiler flags.

Good point. Just don't allow anything for DFLAGS: but a blank line.

I.e. just rename NO_DFLAGS to DFLAGS. (You already must have dealt with the issue of what to do with: NO_DFLAGS: foo).

WalterBright · 2018-02-09T00:43:33Z

It could be argued that we could simplify the test suite by removing all the other test variables and only allowing DFLAGS to be overridden or appended

Not by me.

wilzbach · 2018-02-09T00:46:01Z

Good point. Just don't allow anything for DFLAGS: but a blank line.

I bet this will confuse people and they will waste time wondering why DFLAGS doesn't take effect or even assume that something is checked which isn't.
Please let's use NO_DFLAGS - it's absolutely clear what's going on and can't be accidentally misused.

JinShil · 2018-02-09T01:01:46Z

I bet this will confuse people and they will waste time wondering why DFLAGS doesn't take effect or even assume that something is checked which isn't.
Please let's use NO_DFLAGS - it's absolutely clear what's going on and can't be accidentally misused.

I agree. @WalterBright, can we convince you to keep this PR as is and support NO_DFLAGS for the reason given above?

marler8997 · 2018-02-09T03:00:39Z

test/d_do_test.d

 {
    string file = cast(string)std.file.read(input_file);

+    string ignore;


I would put ignore in it's own scope

{ string ignore; testArgs.noDflags = findTestParameter(envData, file, "NO_DFLAGS", ignore); }

marler8997 · 2018-02-09T03:03:03Z

I wonder how many tests don't need druntime/phobos? Probably the majority correct? Maybe the better solution is to have DFLAGS be "opt-in" instead of "opt-out"?

Would also make it a bit clearer that using druntime/phobos should be the "atypical" case.

JinShil · 2018-02-09T03:36:38Z

I wonder how many tests don't need druntime/phobos?

I bet most, if not all, don't need Phobos, but I'd be willing to bet 99.9% need druntime simply because of object.d. Afterall that's where fundamentals of the language like Object, string, and size_t are declared and defined.

WalterBright · 2018-02-09T03:46:34Z

I bet this will confuse people

How? I set/reset env variables all the time, and setting them to nothing is VAR=. There's nothing confusing about it.

and they will waste time wondering why DFLAGS doesn't take effect or even assume that something is checked which isn't.

If they set DFLAGS=something then they should get an error message. I don't see how that is confusing.

marler8997 · 2018-02-09T04:01:19Z

I bet most, if not all, don't need Phobos, but I'd be willing to bet 99.9% need druntime simply because of object.d.

Ah ok, nevermind then :)

WalterBright · 2018-02-09T04:31:15Z

@wilzbach Keep in mind that you have another PR with Use -conf= to overwrite the default dmd config, not -no_conf :-)

marler8997 · 2018-02-09T04:32:11Z

DFLAGS= works in .sh files, but doesn't work in .d files does it?

JinShil · 2018-02-09T04:34:04Z

Yes, I believe we'll have to use DFLAGS= in .sh files and DFLAGS: in .d files if we go with @WalterBright's preference.

marler8997 · 2018-02-09T04:36:22Z

DFLAGS: for .d files and DFLAGS= for .sh files seems more consistent. But NO_DFLAGS: seems more intuitive than DFLAGS:...sounds like a wash to me

wilzbach · 2018-02-09T08:44:23Z

How? I set/reset env variables all the time, and setting them to nothing is VAR=. There's nothing confusing about it.
...

I was talking about things like:

// DFLAGS: -version=Foo

version(Foo) {
 ... // this is never executed, but assumed by contributor and reviewers to be executed & tested
}

But NO_DFLAGS: seems more intuitive than DFLAGS:...sounds like a wash to me

Maybe it should have been named NO_CONF
Well, there are already these accepted in-test boolean variables:

COMPILE_SEPARATELY (set to enable)
LINK (set to enable)

If they set DFLAGS=something then they should get an error message. I don't see how that is confusing.

Ok. We should do this no matter how the variable is named.

`@wilzbach Keep in mind that you have another PR with Use -conf= to overwrite the default dmd config, not -no_conf

That would imply we should use CONF:, no?
Not a bad idea!

CC @CyberShadow who has always shown a lot of wisdom in such CLI usability questions.

JinShil · 2018-02-09T10:40:31Z

OK, I've implemented DFLAGS:. d_do_test.d will throw an error if it is not empty.

I still need to figure out why this isn't working for Windows though.

wilzbach · 2018-02-09T10:48:11Z

test/d_do_test.d

+        if (testArgs.dflags != "")
+            throw new Exception("The DFLAGS test argument must be empty");
+
+        environment["DFLAGS"] = testArgs.dflags;


Maybe "" for clarity?

JinShil · 2018-02-10T09:40:42Z

Interesting:
Win32: import object (..\..\druntime\import\object.d) --> WRONG!
Win32_64: import path[0] = fail_compilation --> Correct!

@braddr and @rainers, Do you have any ideas why Win32 is still importing object.d in this case (See AutoTester results)

rainers · 2018-02-10T10:27:25Z

Clearing the environment variable with environment["DFLAGS"] = "" doesn't work. Using putenv helps, but the actual issue is probably in std.process.

JinShil · 2018-02-10T14:01:27Z

I tried calling Win32 API function SetEnvironmentVariable, but that didn't work either. @braddr, is the DFLAGS variable set in the "System" environment variables on the Win32 machine?

wilzbach · 2018-02-10T14:19:16Z

FYI: That's all I know about:

https://github.com/braddr/at-client/blob/master/src/do_test_dmd.sh
https://github.com/braddr/at-client/blob/master/src/diff-dmd-win64.diff

marler8997 · 2018-02-10T17:45:42Z

It looks like d_do_test is using the system function to invoke the compiler. I believe that function will just inherit its environment variables from the parent process, however, I don't think setting environment["DFLAGS"] = "" will actually change the process's environment variables. Did you try putenv? If that doesn't work, we could probably use a different function to invoke the compiler (other than system) that could accept environment variables to pass to the child process.

JinShil · 2018-02-10T23:53:47Z

It looks like d_do_test is using the system function to invoke the compiler. I believe that function will just inherit its environment variables from the parent process

Isn't the process executing d_do_test the parent process? If so, the environment variables are cleared there before the call to system, so it should work, at least as I understand it.

Did you try putenv?

putenv is a POSIX function, isn't it? The only platform that doesn't work is Win32. Even Win32_64 works. There's something odd about Win32.

marler8997 · 2018-02-11T00:22:15Z

Ah, it looks like std.process actually does call SetEnvironmentVariable when you set environment["DFLAGS"] = "". Maybe win32 isn't trying to import object.d. Maybe try adding something into the test file that explicitly depends on it?

// string is defined in object.d, this should guarantee that object.d is getting loaded
static assert(string.sizeof > 0);

wilzbach · 2018-02-11T00:25:52Z

test/fail_compilation/no_object.d

@@ -0,0 +1,15 @@
+/*
+DFLAGS:
+REQUIRED_ARGS:


Try -v here to debug win32

(edit: I just tried this for you)

I did. See the result here: https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=3027610&isPull=true

import object (....\druntime\import\object.d)

I don't know where that's coming from.

(edit: I just tried this for you)

You'll probably want to disable other platforms or the auto-tester will fail and will never get around to running the test in question.

Oh sorry. Feel free to drop my commit.
We should really print D_FLAGS with -v too ...

We should really print D_FLAGS with -v too ...

#7865 - this might help you.

rainers · 2018-02-11T08:12:34Z

putenv is a POSIX function, isn't it?

The OMF library snn.lib has putenv, too, it just isn't declared in core.stdc.stdlib. Using putenv("DFLAGS="); with an appropriate declaration worked for me for yesterday.

JinShil · 2018-02-11T15:00:38Z

Thanks @rainers. Using putenv from snn.lib seems to have worked. I'll do some more testing on std.process.environment and either submit a PR to phobos, or file a bug. For now, I've implemented a workaround just for Win32.

This should now be ready to go.

marler8997 · 2018-02-11T19:01:15Z

Should a bug be filed for std.process?

JinShil · 2018-02-11T22:43:38Z

Should a bug be filed for std.process?

Yes. If I can create an isolated test case, I'll file a bug.

JinShil · 2018-02-12T01:48:14Z

Phew! What a chore that turned out to be. I had to make a couple of changes (it worked once and then started failing again... weird). This should be ready to go now.

I'll file a bug for std.process after I get in front of a Windows machine, and assuming I can reproduce the problem in an isolated test.

marler8997 · 2018-02-12T06:06:20Z

I've created an issue so we don't forget to address this: https://issues.dlang.org/show_bug.cgi?id=18425

rainers · 2018-02-12T07:32:58Z

test/d_do_test.d

+        version(Win32)
+        {
+            char[] varExpression = cast(char[])"DFLAGS=\0";
+            putenv(varExpression.ptr);


Declaring extern(C) int putenv(const char *); globally and calling putenv("DFLAGS="); worked for me. The literal already has the trailing zero and converts implicitly.

rainers · 2018-02-12T07:39:04Z

I suspect the issue is that SetEnvironmentVariable is called with an empty string, while removal should be done with a null argument. Not sure why this works for Win64, though.

marler8997 · 2018-02-12T07:44:31Z

Hmmm, but deleting DFLAGS or setting it to an empty string should still produce the same result...

JinShil · 2018-02-12T07:54:26Z

I suspect the issue is that SetEnvironmentVariable is called with an empty string, while removal should be done with a null argument

I tried calling SetEnvironmentVariable directly with both null and empty strings, and neither worked.

marler8997 · 2018-02-12T07:56:50Z

Might have something to do with mixing native windows functions with posix. The system posix function may not pick up environment variables from SetEnvironmentVariable

rainers · 2018-02-12T07:58:43Z

Hmmm, but deleting DFLAGS or setting it to an empty string should still produce the same result...

Maybe the empty string is silently ignored. Both snn.lib and libcmt.lib implement putenv via SetEnvironmentVariable and convert it to the null parameter for removal.

wilzbach · 2018-02-12T21:23:08Z

Thanks a lot @JinShil @rainers @marler8997 for this diligent debugging work. We should really look into fixing std.process :O

wilzbach mentioned this pull request Feb 6, 2018

Added -Xi to include new fields in JSON output #7838

Merged

JinShil added the Review:Easy Review label Feb 6, 2018

wilzbach reviewed Feb 8, 2018

View reviewed changes

wilzbach approved these changes Feb 8, 2018

View reviewed changes

wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Feb 8, 2018

marler8997 approved these changes Feb 9, 2018

View reviewed changes

wilzbach removed the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Feb 9, 2018

wilzbach reviewed Feb 9, 2018

View reviewed changes

WalterBright approved these changes Feb 9, 2018

View reviewed changes

wilzbach reviewed Feb 11, 2018

View reviewed changes

wilzbach mentioned this pull request Feb 11, 2018

Print DFLAGS on -v to ease debugging #7865

Merged

JinShil removed the Review:WIP Work In Progress - not ready for review or pulling label Feb 12, 2018

rainers reviewed Feb 12, 2018

View reviewed changes

Add ability to override DFLAGS env variable in test suite

b3bb5f8

wilzbach approved these changes Feb 12, 2018

View reviewed changes

wilzbach merged commit 7103c93 into dlang:master Feb 12, 2018

JinShil mentioned this pull request Feb 13, 2018

Fix no_Throwable test so it doesn't import druntime's object.d #7883

Merged

JinShil mentioned this pull request Mar 20, 2018

Fix issue 18242 - Warn if object.Exception is missing #8050

Merged

JinShil deleted the dflags branch April 24, 2018 10:24

	DRUNTIME_PATH=../../druntime
	PHOBOS_PATH=../../phobos
	# link against shared libraries (defaults to true on supported platforms, can be overridden w/ make SHARED=0)
	SHARED=$(if $(findstring $(OS),linux freebsd),1,)
	DFLAGS=-I$(DRUNTIME_PATH)/import -I$(PHOBOS_PATH) -L-L$(PHOBOS_PATH)/generated/$(OS)/$(BUILD)/$(MODEL)

Uh oh!

Comments

Conversation

JinShil commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Feb 6, 2018

Bugzilla references

Uh oh!

wilzbach commented Feb 6, 2018

Uh oh!

wilzbach commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JinShil commented Feb 6, 2018

Uh oh!

wilzbach commented Feb 6, 2018

Uh oh!

JinShil commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

WalterBright commented Feb 8, 2018

Uh oh!

JinShil commented Feb 8, 2018

Uh oh!

WalterBright commented Feb 9, 2018

Uh oh!

WalterBright commented Feb 9, 2018

Uh oh!

wilzbach commented Feb 9, 2018

Uh oh!

JinShil commented Feb 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marler8997 commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JinShil commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WalterBright commented Feb 9, 2018

Uh oh!

marler8997 commented Feb 9, 2018

Uh oh!

WalterBright commented Feb 9, 2018

Uh oh!

marler8997 commented Feb 9, 2018

Uh oh!

JinShil commented Feb 9, 2018

Uh oh!

marler8997 commented Feb 9, 2018

Uh oh!

wilzbach commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JinShil commented Feb 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rainers commented Feb 10, 2018

Uh oh!

JinShil commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Feb 10, 2018

Uh oh!

marler8997 commented Feb 10, 2018

Uh oh!

JinShil commented Feb 10, 2018

JinShil commented Feb 6, 2018 •

edited

Loading

wilzbach commented Feb 6, 2018 •

edited

Loading

JinShil commented Feb 8, 2018 •

edited

Loading

wilzbach Feb 8, 2018 •

edited

Loading

marler8997 commented Feb 9, 2018 •

edited

Loading

JinShil commented Feb 9, 2018 •

edited

Loading

wilzbach commented Feb 9, 2018 •

edited

Loading

JinShil commented Feb 10, 2018 •

edited

Loading

JinShil commented Feb 10, 2018 •

edited

Loading

marler8997 commented Feb 11, 2018 •

edited

Loading

wilzbach Feb 11, 2018 •

edited

Loading