ghc, ghc-pkg: use getExecutablePath on Windows when base >= 4.11.0
ClosedPublic

Authored by Phyx on Nov 23 2017, 10:39 AM.

Details

Summary

This completes the work started in D4227 by using just 'getExecutablePath'
in ghc and ghc-pkg when building with base >= 4.11.0.

On the long term, we will be able to simply kill the existing code that
follows (or not) symlinks and just get this behaviour for free from
getExecutable. For now we however have to require base >= 4.11.0 to be able
to just use getExecutablePath under Windows, and use the current code when
building with an older base.

Original code by @alpmestan commandeering since patch has been stale
and bug remains open.

Test Plan

Validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
alpmestan created this revision.Nov 23 2017, 10:39 AM
alpmestan updated this revision to Diff 14783.Nov 23 2017, 10:46 AM
  • factor out 'rootDir'
bgamari requested changes to this revision.Nov 23 2017, 12:03 PM
bgamari added inline comments.
compiler/main/SysTools.hs
393

A comment would be nice.

utils/ghc-pkg/Main.hs
2217–2218

Can we have a comment explaining what this controls and why it's needed?

This revision now requires changes to proceed.Nov 23 2017, 12:03 PM
alpmestan updated this revision to Diff 14792.Nov 24 2017, 4:18 AM
  • some explanatory comments
alpmestan marked 2 inline comments as done.Nov 24 2017, 4:23 AM

Added some comments and asked a question to @Phyx about an opportunity for simplifying the code in SysTools.hs a little bit.

compiler/main/SysTools.hs
396

I'm wondering whether we could simply do without this rootDir function altogether and just use takeDirectory . takeDirectory, which is what the unix/osx implementation does, and this also appears to be what we do _on Windows_ in ghc-pkg's Main module (see my changes to that file). @Phyx It seems to me that this is all equivalent, modulo the call to normalise. Is that true?

Phyx added inline comments.Nov 24 2017, 4:42 AM
compiler/main/SysTools.hs
396

The checking of the executable name can go, but the more important call is to normalise, as this gets the path into mixed mode, which means \ don't need to be escaped before being passed on to external utilities. Such as configure

alpmestan added inline comments.Nov 24 2017, 4:46 AM
compiler/main/SysTools.hs
396

So takeDirectory . takeDirectory . normalise would do?

Phyx added inline comments.Nov 24 2017, 1:40 PM
compiler/main/SysTools.hs
396

Oops, I guess my email didn't go through, Yes I think it should do.

bgamari requested changes to this revision.Nov 27 2017, 1:27 PM

It looks like this doesn't validate on Windows.

This revision now requires changes to proceed.Nov 27 2017, 1:27 PM
alpmestan updated this revision to Diff 14845.Nov 29 2017, 4:40 AM
  • simplify rootDir
  • import getExecutablePath on Windows too
alpmestan updated this revision to Diff 14846.Nov 29 2017, 6:50 AM
  • fix redundant imports
alpmestan updated this revision to Diff 14847.Nov 29 2017, 9:15 AM
  • factor out 'rootDir'
  • some explanatory comments
  • simplify rootDir
  • import getExecutable on Windows too, when when can use it
  • fix redundant imports
  • rebase on top of master

I've addressed the feedback and rebased on top of master. Let's see if harbomaster is happy with that.

alpmestan updated this revision to Diff 14860.Nov 30 2017, 2:50 AM
  • SysTools.BaseDir: import getExecutablePath on Windows too, fix rootDir typo
alpmestan updated this revision to Diff 14861.Nov 30 2017, 10:39 AM
  • fix the windows, base >= 4.11 implementation of getBaseDir
alpmestan updated this revision to Diff 14865.Dec 1 2017, 3:12 AM
  • redundant imports
alpmestan updated this revision to Diff 14866.Dec 1 2017, 5:11 AM
  • conditionals for redundant import warning
Phyx added inline comments.Dec 4 2017, 5:43 PM
utils/ghc-pkg/Main.hs
34

Any particular reason why this can't just be #define SIMPLE_WIN_GETLIBDIR (defined(mingw32_HOST_OS) && MIN_VERSION_base(4,11,0)) ?

alpmestan added inline comments.Dec 5 2017, 9:51 AM
utils/ghc-pkg/Main.hs
34

That was mostly because of line length IIRC. I have an emacs mode that highlights when you go beyond some number of columns, etc.

Phyx added inline comments.Dec 5 2017, 3:24 PM
utils/ghc-pkg/Main.hs
34

CPP can be broken up in mltiple lines with \.

#define SIMPLE_WIN_GETLIBDIR (defined(mingw32_HOST_OS) \
                             && MIN_VERSION_base(4,11,0))

for instance

alpmestan added inline comments.Dec 6 2017, 12:58 PM
utils/ghc-pkg/Main.hs
34

Absolutely, I just typed that in on the spot, but will define the macro in a single spot, as soon as I have figured out why/how my patch making the (Windows) build fail.

Did we ever open a ticket summarizing the status of this, @alpmestan?

Phyx requested changes to this revision.Dec 19 2017, 3:26 PM

It's failing at stage2, where SIMPLE_WIN_GETLIBDIR would be 1, so this means what you're getting back from getLibDir is likely not correct. I'm suspicious of the takeDirectory . takeDirectory part. Given our current layout <dir>/inplace/bin/ghc.exe taking a double takeDirectory would get you to <dir>/lib instead of <dir>/inplace/lib as we want. This would explain the error of not finding the settings file.

This revision now requires changes to proceed.Dec 19 2017, 3:26 PM
In D4229#119197, @Phyx wrote:

It's failing at stage2, where SIMPLE_WIN_GETLIBDIR would be 1, so this means what you're getting back from getLibDir is likely not correct. I'm suspicious of the takeDirectory . takeDirectory part. Given our current layout <dir>/inplace/bin/ghc.exe taking a double takeDirectory would get you to <dir>/lib instead of <dir>/inplace/lib as we want. This would explain the error of not finding the settings file.

Probably needs a guard for Hadrian or Makefile build. As the reloc hadrian Logic consistently only has bin and lib next to each other and no inplace anymore.

Sorry for the lack of reactivity on this diff, I somehow didn't get the notifications for all the comments.

Did we ever open a ticket summarizing the status of this, @alpmestan?

I did not, but it's quite simple: my code apparently isn't correct, causing build errors on (the Windows build of) harbormaster that I have not diagnosed yet. Shall I create a ticket to track the status of this patch?

In D4229#119197, @Phyx wrote:

It's failing at stage2, where SIMPLE_WIN_GETLIBDIR would be 1, so this means what you're getting back from getLibDir is likely not correct. I'm suspicious of the takeDirectory . takeDirectory part. Given our current layout <dir>/inplace/bin/ghc.exe taking a double takeDirectory would get you to <dir>/lib instead of <dir>/inplace/lib as we want. This would explain the error of not finding the settings file.

The takeDirectory . takeDirectory part was already there. I also just checked in ghci:

Prelude λ> import System.FilePath
Prelude System.FilePath λ> (</> "lib") . takeDirectory . takeDirectory $ "inplace/bin/ghc-stage2.exe"
"inplace/lib"

which is where we want to look to find the settings file, right? At least I have one there. So the problem must come from elsewhere I suppose? I just haven't been able to see it so far.

Probably needs a guard for Hadrian or Makefile build. As the reloc hadrian Logic consistently only has bin and lib next to each other and no inplace anymore.

Hmm. So in (your branch of) hadrian we have <build dir>/stage<n>/{bin, lib} while with the make build system we have ./inplace/{bin, lib} when building/running in place and... pretty much any structure we want when installing to some place on the system, except that in that case it's not a problem because we place the actual binaries in a fixed place and generate wrapper scripts that pass the right -B/path/to/libdir to GHC, in which case GHC doesn't have to look at some place relative to the path of the program (or symlink) through which it's invoked etc. So long story short, I don't think we need a different logic, I just have to find and fix one or more subtle issue(s) with my patch. Unless I'm missing something?

Phyx added a comment.Jul 26 2018, 1:58 PM

@alpmestan what's the status of this? without it the fix for Trac #14460 is incomplete.

I didn't get around to figuring out the source of the Windows CI troubles. I can put this patch back on my radar and look into this in the upcoming days.

alpmestan updated this revision to Diff 17464.Jul 27 2018, 4:27 AM
  • Merge branch 'master' into wip/alp/14483-getexecutablepath-windows
Phyx requested changes to this revision.Jul 27 2018, 6:21 PM

@alpmestan Thanks for picking it up again!

Think you need to rebase this on HEAD to fix the Linux build and also you're missing an import for doesDirectoryExist.

This revision now requires changes to proceed.Jul 27 2018, 6:21 PM
Phyx commandeered this revision.Aug 11 2018, 1:20 PM
Phyx edited reviewers, added: alpmestan; removed: Phyx.
Phyx updated this revision to Diff 17637.Aug 11 2018, 1:34 PM
  • fix build issues and rebase
Phyx edited the summary of this revision. (Show Details)Aug 12 2018, 7:12 AM
Phyx edited the summary of this revision. (Show Details)
bgamari accepted this revision.Aug 23 2018, 4:03 PM

Looks great. Thanks for the ping.

This revision is now accepted and ready to land.Aug 23 2018, 4:03 PM
This revision was automatically updated to reflect the committed changes.