Skip to content

Comments

Improve the lookup sequence for finding the config file#7915

Closed
wilzbach wants to merge 1 commit intodlang:masterfrom
wilzbach:conf
Closed

Improve the lookup sequence for finding the config file#7915
wilzbach wants to merge 1 commit intodlang:masterfrom
wilzbach:conf

Conversation

@wilzbach
Copy link
Contributor

In short, it's very problematic that ~/dmd.conf has a higher precedence than the config next to the dmd binary.
This easily leads to unexpected behavior.
I think @MartinNowak has a very good explanation at #4256 (comment):


tl;dr, please reconsider changing the conf order or splitting the conf file

That dmd.conf is driving me crazy.

I need a dmd.conf in my dmd repo, so that I can use dmd-master. It wouldn't work without a config and putting dmd.conf anywhere but near dmd itself overrides my system dmd.conf.
That setup worked nicely for me although some of you guys seem to have a dmd.conf in your home dir (why?).

Now that we require a host D compiler to build dmd it no longer works, because the dmd.conf for my dmd-master overrides the system wide dmd.conf of my host dmd.
Why would my system dmd need a different configuration just because I am in a different folder?
It doesn't, it still uses the same phobos (the system wide) and still requires the same linker switches.

Setting HOST_DC is a workaround but is impractical, because it requires an awkward env HOST_DC='dmd -conf=/etc/dmd.conf' make -f posix.mak to work.
Requiring dmd -conf=/etc/dmd.conf in order to not pickup some random conf file is crazy. It's not portable either, because the config is somewhere else on each platform.

We learned the same from dlang.org dlang/dlang.org#758 (comment), adding the -conf= switch didn't solve the actual problem and created new ones.

It might seem intuitive, that a local configuration should override a global one, but it's a false friend here, because the configuration should be local from the perspective of the compiler, not from the perspective of the user.

In fact the config is so tied to the compiler that we could almost compile it into the compiler (like gcc does with it's spec). At best it's something package maintainers need to touch to accomodate platform differences, but it's not a user configuration file.

If people use it to configure DFLAGS and such project dependent stuff, then we need to split the configuration file, into one part that tells the compiler where to find druntime/phobos and how to link, and one part that can be used for per-project configuration of compiler arguments and env variables.
IMO the latter is better kept in makefiles/dub.json though.

conclusion

The lookup order for the config file should be changed to the following.

  • next to dmd binary (highest precedence so that I can have multiple installations)
  • per-user conf folder (HOME) (to override the system-wide config)
  • system-wide conf folder (to allow package installations .deb/.rpm)

The current situation is unmaintainable.


LDC's search oder

FWIW I think LDC's search order is nice and we should learn from them:

  • current working directory
  • next to binary
  • ~/.ldc
  • user home directory (Windows-only)
  • in an etc directory next to the directory where the binary resides
  • install-prefix (Windows-only)
  • install-prefix/etc
  • install-prefix/etc/ldc
  • /etc
  • /etc/ldc

Other path DMD should look at:

Resulting issues I found on a quick scan:

As you can see the current status quo is a mess. I'm more than happy to fix the two open issues in this PR too. Here's the order I suggest

  • current working directory
  • exe directory (windows)
  • directory of argv0
  • ~/.dmd.conf
  • ~/dmd.conf (legacy :/)
  • user home directory (Windows-only)
  • install-prefix/etc/dmd.conf
  • SYSCONFDIR=/etc (non-windows)

@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.


$(OL
$(LI current directory)
$(LI exe directory (windows))
Copy link
Contributor

Choose a reason for hiding this comment

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

To me this looks like the exe directory is not checked at all on non-Windows platforms. That's not correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT the argv0 directory is pretty much the exe directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right, not sure what I was thinking.

@jacob-carlborg
Copy link
Contributor

Do we have tests for this? Can we test this?

@wilzbach
Copy link
Contributor Author

Do we have tests for this?

I don't think so.

Can we test this?

It's a bit tricky within the testsuite as -conf= is the default DFLAG, but we could:

  • do symlinking and check that the current directory takes precendence over the binary directory
  • add a few lines to the CircleCi config to check, e.g. /etc/dmd.conf lookup

Anyhow before investing more work into this, I would like to make sure that everyone is okay with improving the lookup order.

@jacob-carlborg
Copy link
Contributor

Anyhow before investing more work into this, I would like to make sure that everyone is okay with improving the lookup order.

Fair enough. I would vote for this new order.

JinShil
JinShil previously approved these changes Feb 28, 2018
@JinShil JinShil dismissed their stale review February 28, 2018 01:50

Approval was the wrong thing to do here. What I mean is I support the new lookup rules.

@WalterBright
Copy link
Member

Generally, people are not going to like breaking their setups. They may not even know what they are - it'll just start failing.
To change the lookup rules, a new filename will be needed.

@AndrewEdwards
Copy link
Contributor

@wilzbach since this was generated to address the concerns @MartinNowak voiced on dlang/installer#297, can you clean it up (rebase and resolve all conflicts) and address @WalterBright's concern? @WalterBright is the filename the only thing holding up this pull request?

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.

7 participants