Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dropExeExtension on Windows #6287

Merged
merged 1 commit into from
Oct 9, 2019
Merged

Conversation

DanielG
Copy link
Collaborator

@DanielG DanielG commented Oct 6, 2019

On Windows dropExeExtension doesn't actually drop the exe extension.

We have

splitExtension "foo.exe" == ("foo", ".exe")

but the code is expecting just "exe" for the extension field.

Among other things this makes guessToolFromGhcPath behave
unexpectedly. Since takeVersionSuffix can't see past the extension the
null suf case in mkGuesses will trigger which causes the versioned
program path candidates to not be looked for.


  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.

Windows -> ["", "exe"]
Ghcjs -> ["", "exe"]
Windows -> ["", ".exe"]
Ghcjs -> ["", ".exe"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So technically this is breaking API compatibility since this is exported, but there don't seem to be any usages outside of Cabal oh hackage: https://codesearch.aelve.com/haskell/search?query=exeExtensions&insensitive=off&space=off&precise=on&sources=on

Furthermore <.> doesn't care about the leading dot in the extension, so "foo" <.> "bar" == "foo" <.> ".bar" == "foo.bar".

@phadej
Copy link
Collaborator

phadej commented Oct 7, 2019

tests/UnitTests/Distribution/Simple/Utils.hs:12:27:
    Module `System.Directory' does not export `exeExtension'

on old GHCs. There's exeExtension in Distribution.Simple.BuildPaths though

EDIT: which returns just "exe" on Windows.

@DanielG
Copy link
Collaborator Author

DanielG commented Oct 7, 2019

Argh, more CPP needed :) nvm, I can just always use Distribution.Simple.BuildPaths.exeExtension.

@DanielG
Copy link
Collaborator Author

DanielG commented Oct 7, 2019

How many revisions can a PR that essentially adds a "." possibly need!? ;-)

@phadej
Copy link
Collaborator

phadej commented Oct 7, 2019 via email

@DanielG
Copy link
Collaborator Author

DanielG commented Oct 7, 2019

Well pretty much any codebase has that property ;) That's why I try to shoot patches as soon as I find issues.

@phadej
Copy link
Collaborator

phadej commented Oct 7, 2019

tests\UnitTests\Distribution\Simple\Utils.hs:92:33: error:
    * Couldn't match type `Distribution.System.Platform -> String'
                     with `[Char]'
      Expected type: String
        Actual type: Distribution.System.Platform -> String
    * Probable cause: `exeExtension' is applied to too few arguments
      In the second argument of `(<.>)', namely `exeExtension'
      In the first argument of `dropExeExtension', namely
        `("foo" <.> exeExtension)'
      In the first argument of `(==)', namely
        `dropExeExtension ("foo" <.> exeExtension)'
   |
92 |     dropExeExtension ("foo" <.> exeExtension) == "foo"
   |                                 ^^^^^^^^^^^^
cabal.exe: Failed to build test:unit-tests from Cabal-3.1.0.0.

@DanielG DanielG force-pushed the fix-drop-exe-ext branch 3 times, most recently from fa1caba to 4527ed5 Compare October 8, 2019 08:50
@DanielG DanielG force-pushed the fix-drop-exe-ext branch 5 times, most recently from aef8a25 to 32e651d Compare October 9, 2019 11:45
@DanielG
Copy link
Collaborator Author

DanielG commented Oct 9, 2019

Travis is green except for Mac, with one timeout and one network error :)

On Windows dropExeExtension doesn't actually drop the exe extension.

We have

    splitExtension "foo.exe" == ("foo", ".exe")

but the code is expecting just "exe" for the extension field.

Among other things this makes guessToolFromGhcPath behave
unexpectedly. Since takeVersionSuffix can't see past the extension the
`null suf` case in mkGuesses will trigger which causes the versioned
program path candidates to not be looked for.
@DanielG DanielG merged commit 0ad0ac0 into haskell:master Oct 9, 2019
DanielG added a commit to DanielG/cabal that referenced this pull request Oct 18, 2019
On Windows dropExeExtension doesn't actually drop the exe extension.

We have

    splitExtension "foo.exe" == ("foo", ".exe")

but the code is expecting just "exe" for the extension field.

Among other things this makes guessToolFromGhcPath behave
unexpectedly. Since takeVersionSuffix can't see past the extension the
`null suf` case in mkGuesses will trigger which causes the versioned
program path candidates to not be looked for.
phadej pushed a commit to phadej/cabal that referenced this pull request Dec 12, 2019
On Windows dropExeExtension doesn't actually drop the exe extension.

We have

    splitExtension "foo.exe" == ("foo", ".exe")

but the code is expecting just "exe" for the extension field.

Among other things this makes guessToolFromGhcPath behave
unexpectedly. Since takeVersionSuffix can't see past the extension the
`null suf` case in mkGuesses will trigger which causes the versioned
program path candidates to not be looked for.
phadej added a commit to phadej/cabal that referenced this pull request Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants