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

Don't create spurious dirs on init #7262

Merged
merged 3 commits into from
Feb 8, 2021

Conversation

emilypi
Copy link
Member

@emilypi emilypi commented Jan 28, 2021

This gets rid of that pesky exe dir being created when we specify libraries interactively:

λ P temp → λ git ignore-app-dir-for-libraries → ../../dist-newstyle/build/x86_64-osx/ghc-8.10.3/cabal-install-3.5.0.0/x/cabal/noopt/build/cabal/cabal init 
...
What does the package build:
   1) Executable
   2) Library
   3) Library and Executable
Your choice? 2
...
Generating LICENSE...
Generating CHANGELOG.md...
Generating src/MyLib.hs...
Generating test/MyLibTest.hs...
Generating temp.cabal...

Warning: no synopsis given. You should edit the .cabal file and add one.
You may want to edit the .cabal file and add a Description field.
λ P temp → λ git ignore-app-dir-for-libraries → ls
CHANGELOG.md LICENSE      src          temp.cabal   test

and for Libraries + Executables,

λ P temp → λ git ignore-app-dir-for-libraries → ../../dist-newstyle/build/x86_64-osx/ghc-8.10.3/cabal-install-3.5.0.0/x/cabal/noopt/build/cabal/cabal init 
Should I generate a simple project with sensible defaults? [default: y] n
What does the package build:
   1) Executable
   2) Library
   3) Library and Executable
Your choice? 3
...
Generating LICENSE...
Generating CHANGELOG.md...
Generating src/MyLib.hs...
Generating exe/Main.hs...
Generating test/MyLibTest.hs...
Generating temp.cabal...

Warning: no synopsis given. You should edit the .cabal file and add one.
You may want to edit the .cabal file and add a Description field.
λ P temp → λ git ignore-app-dir-for-libraries → ls
CHANGELOG.md LICENSE      exe          src          temp.cabal   test

and for Executables,

λ P temp → λ git ignore-app-dir-for-libraries* → ../../dist-newstyle/build/x86_64-osx/ghc-8.10.3/cabal-install-3.5.0.0/x/cabal/noopt/build/cabal/cabal init
Should I generate a simple project with sensible defaults? [default: y] n
What does the package build:
   1) Executable
   2) Library
   3) Library and Executable
Your choice? 1
...
Generating LICENSE...
Generating CHANGELOG.md...
Generating exe/Main.hs...
Generating temp.cabal...

Warning: no synopsis given. You should edit the .cabal file and add one.
You may want to edit the .cabal file and add a Description field.
λ P temp → λ git ignore-app-dir-for-libraries* → ls
CHANGELOG.md LICENSE      exe          temp.cabal

Please include the following checklist in your PR:

@emilypi
Copy link
Member Author

emilypi commented Jan 28, 2021

Question about the changelog: master seems to only be as recent as 3.2. Do you want to just cherrypick this, or should i add an entry?

@emilypi emilypi force-pushed the ignore-app-dir-for-libraries branch from 990a49d to a04be32 Compare January 28, 2021 03:08
@fgaz
Copy link
Member

fgaz commented Jan 28, 2021

master seemes to only be as recent as 3.2

@emilypi what do you mean?

@emilypi
Copy link
Member Author

emilypi commented Jan 28, 2021

@fgaz the changelog for cabal-install ends with 3.2. It doesn't seem to have been maintained. https://github.com/haskell/cabal/blob/master/cabal-install/changelog

@fgaz
Copy link
Member

fgaz commented Jan 28, 2021

I think that's because 3.2 is still the last release and 3.4 is still at the rc stage. @phadej?

@emilypi emilypi force-pushed the ignore-app-dir-for-libraries branch 2 times, most recently from 9c1d466 to f5ca335 Compare January 28, 2021 19:15
@@ -104,7 +134,9 @@ exeFlags = baseFlags {
-- Create an executable only, with main living in app/Main.hs.
packageType = Flag Executable
, mainIs = Flag "Main.hs"
, sourceDirs = Just []
Copy link
Member Author

Choose a reason for hiding this comment

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

sourceDirs = Nothing in baseFlags was actually the wrong state for this to be in. Throughout the course of init it actually gets turned into Just [] for Executables. So, to test against this, we set this as our golden data.

@emilypi emilypi force-pushed the ignore-app-dir-for-libraries branch 3 times, most recently from 334e464 to b60517d Compare January 28, 2021 21:19
cabal-install/TESTING.md Outdated Show resolved Hide resolved
@emilypi emilypi force-pushed the ignore-app-dir-for-libraries branch from 3ae0c57 to 89a5c41 Compare January 28, 2021 22:10
@fgaz
Copy link
Member

fgaz commented Jan 28, 2021

Could you split the TESTING.md stuff into a separate commit?

@emilypi emilypi force-pushed the ignore-app-dir-for-libraries branch 2 times, most recently from 156e1d4 to 3a6d592 Compare January 28, 2021 22:23
@emilypi emilypi force-pushed the ignore-app-dir-for-libraries branch from b6b1266 to 1a06300 Compare January 30, 2021 17:30
@emilypi emilypi mentioned this pull request Jan 30, 2021
@emilypi
Copy link
Member Author

emilypi commented Feb 1, 2021

@fgaz split up for you.

@emilypi emilypi force-pushed the ignore-app-dir-for-libraries branch 2 times, most recently from 1dc26ff to 8319496 Compare February 1, 2021 19:03
@emilypi
Copy link
Member Author

emilypi commented Feb 1, 2021

I found a few more bugs while going through the code and wrote up a proposal fix for #7265. In case that doesn't get done by the time 3.4 is released, I think this would still be a good quality of life improvement for this particular function.

Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

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

Thanks for splitting the commit!
Please replace the current subject lines of the commit messages (commit x of n) with short summaries of the changes

cabal-install/cabal-install.cabal.dev Show resolved Hide resolved
@emilypi emilypi force-pushed the ignore-app-dir-for-libraries branch 2 times, most recently from c79e5ef to a940fc4 Compare February 1, 2021 22:54
@emilypi emilypi force-pushed the ignore-app-dir-for-libraries branch from a940fc4 to e5d57ad Compare February 1, 2021 22:57
@emilypi
Copy link
Member Author

emilypi commented Feb 8, 2021

@fgaz can I get a 👍 ?

Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

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

👍 👍

Remember to add the changelog entry before merging!

@emilypi
Copy link
Member Author

emilypi commented Feb 8, 2021

Thanks @fgaz! Added, and merging.

@emilypi emilypi merged commit 9e6b7f3 into haskell:master Feb 8, 2021
@emilypi emilypi deleted the ignore-app-dir-for-libraries branch February 8, 2021 21:13
@emilypi emilypi mentioned this pull request Feb 8, 2021
3 tasks
@ulysses4ever
Copy link
Collaborator

Does this merge mean that #6772 is fixed (in master) now? Should it be closed?

@emilypi
Copy link
Member Author

emilypi commented May 19, 2021

Yes, good call @ulysses4ever

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.

3 participants