Skip to content

Comments

Use -conf= to overwrite the default dmd config#7846

Closed
wilzbach wants to merge 2 commits intodlang:masterfrom
wilzbach:conf-=
Closed

Use -conf= to overwrite the default dmd config#7846
wilzbach wants to merge 2 commits intodlang:masterfrom
wilzbach:conf-=

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Feb 6, 2018

-conf= should almost always be used in any of our Makefiles to avoid any issues with user-defined configuration files.

Note: there's currently an permananet failure on some CIs. See also #7838 (comment)

@dlang-bot
Copy link
Contributor

dlang-bot commented Feb 6, 2018

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#7846"

@JinShil
Copy link
Contributor

JinShil commented Feb 6, 2018

-conf= is already set in the execution of each test. See

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

@marler8997
Copy link
Contributor

-conf= is already set in the execution of each test...

I believe that only affects .d tests, not .sh tests. Setting it in the Makefile should affect ALL tests.

@JinShil
Copy link
Contributor

JinShil commented Feb 9, 2018

Then, shouldn't this PR also remove conf= from d_do_test.d?

@marler8997
Copy link
Contributor

Then, shouldn't this PR also remove conf= from d_do_test.d?

I can't think of a reason that d_do_test would need to redundantly set -conf=, so I suppose that should work.

@marler8997
Copy link
Contributor

One thought (somewhat unrelated). Sometimes it's nice to be able to copy the command line and run it on it's own (add/remove arguments like -v) and the more arguments that depend on the Makefile setting up the environment, the more arguments you have to manually add back into the command line. Maybe redundancy is ok in this case.

@wilzbach wilzbach added the Review:WIP Work In Progress - not ready for review or pulling label Feb 9, 2018
@wilzbach wilzbach force-pushed the conf-= branch 2 times, most recently from 596e2ea to feef3f0 Compare March 26, 2018 16:05
@wilzbach wilzbach added Infrastructure:Continuous Integration and removed Review:WIP Work In Progress - not ready for review or pulling labels Mar 26, 2018
echo 'void main() {}' > "${src}"

# Only compile, not link, since optlink can't handle long file names
$DMD -m"${MODEL}" "${DFLAGS}" -c -of"${bin}" "${src}"
Copy link
Contributor Author

@wilzbach wilzbach Mar 26, 2018

Choose a reason for hiding this comment

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

It should be "${DFLAGS[@]}" if expansion to multiple arguments is wanted, but DFLAGS is already an environment variable, so this isn't needed.
BTW it might be worthwhile to set -m"${MODEL}" in dflags too to avoid accidentally forgetting -m"${MODEL}" when it matters in the bash scripts.

@wilzbach
Copy link
Contributor Author

I can't think of a reason that d_do_test would need to redundantly set -conf=, so I suppose that should work.

I left it in d_do_test because the environment flags are printed on a failure.
So when a test failed, you get this nice output with all parameters that were used to run the file:

Test compilable/test10056.d failed.  The logged output:
../generated/linux/release/64/dmd -conf= -m64 -Icompilable  -fPIC  -odtest_results/compilable -oftest_results/compilable/test10056_0.o  -c compilable/test10056.d 

This isn't so important for the bash scripts, because you can't run them directly anyways and you see all variables used thanks to xtrace.

@marler8997
Copy link
Contributor

I left it in d_do_test because the environment flags are printed on a failure.
So when a test failed, you get this nice output with all parameters that were used to run the file

It doesn't print the "environment variables" though...does it?

@wilzbach
Copy link
Contributor Author

Nope and we shouldn't be depending on the user to set environment variables to repeat the run of a test. I know in theory we could do DFLAGS="..." generated..., but that I think the current behavior of d_do_test is rather nice.
BTW d_do_test enforces DFLAGS to be an empty string to avoid exactly these problems:

throw new Exception("The DFLAGS test argument must be empty: It is '" ~ testArgs.dflags ~ "'");

So the "redundant" -conf= is really required.

@wilzbach wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Mar 27, 2018
@JinShil JinShil removed the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Apr 5, 2018
@JinShil
Copy link
Contributor

JinShil commented Apr 5, 2018

Then, shouldn't this PR also remove conf= from d_do_test.d?

Please address this.

@Geod24
Copy link
Member

Geod24 commented Aug 31, 2018

Ping @wilzbach

@RazvanN7
Copy link
Contributor

The Makefile for running tests is deprecated. Closing this.

@RazvanN7 RazvanN7 closed this Jan 21, 2022
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