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

Disable .ghc-environment file generation by default #5985

Merged
merged 1 commit into from Apr 7, 2019

Conversation

mpickering
Copy link
Collaborator

See https://mail.haskell.org/pipermail/ghc-devs/2019-March/017351.html for discussion about this default.


Please include the following checklist in your PR:

  • 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.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

Copy link
Member

@hvr hvr left a comment

Choose a reason for hiding this comment

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

While I still consider most of the arguments brought forward for changing the default to be weak and in some cases even irrational, and I therefore remain unconvinced by those, I'm willing to reconsider the default setting for the upcoming 3.0 release; it's not like like we can't change the default back once we've worked out the few remaining kinks.

@hvr hvr added this to the 3.0 milestone Apr 5, 2019
Copy link
Member

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

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

LGTM modulo comments.

cabal-install/changelog Outdated Show resolved Hide resolved
cabal-install/Distribution/Client/Types.hs Outdated Show resolved Hide resolved
@mpickering mpickering force-pushed the disable-ghc-environment-files branch from 1de83f4 to 6089506 Compare April 5, 2019 13:05
@mpickering
Copy link
Collaborator Author

@23Skidoo I made your suggested changes.

@vaibhavsagar
Copy link
Collaborator

Thank you for doing this @mpickering!

@hvr
Copy link
Member

hvr commented Apr 5, 2019

Just as a reminder, we need to make a couple of follow-up changes to prevent bad user-experience as a result of this change of defaults, as ironically changing the default this way will guarantee a couple of regression for cabal users if caught off guard.

@gbaz
Copy link
Collaborator

gbaz commented Apr 5, 2019

Let me expand on that -- in an irc discussion with someone who had been quite frustrated with env files, it turns out the problem was precisely that we'd limited their auto generation to 8.4.4 and after. Hence, they had files for 8.4.3, etc projects that were out of date, and which weren't getting auto-regenerated (due to changing defaults), and this was what made the env files fail for them.

So, in particular, we need to handle the case where someone updates cabal, then env files stop being generated, and then workflows break without their realizing it.

What this suggests to me is we need some behavior when env files already exist that A) detects them and B) either warns, offers to delete, or says "if they already exist, then update them" -- i.e. the default is not "don't create or update" but rather "don't create, but do update".

@hsenag
Copy link
Member

hsenag commented Apr 5, 2019

"don't create but do update" sounds good for the long-term behaviour as it's then opt-in - particularly if it can detect the cabal-generated files and only update those and not one created directly by a user.

In common with many others I'm against the current behaviour: it's both a layering violation (a high-level tool cabal modifying the behaviour of a low-level tool ghci) and a silent side-effect.

@kcsongor
Copy link

kcsongor commented Apr 6, 2019

@gbaz I like your proposal about issuing a warning when the environment file is not being generated, as I've run into a similar situation myself.
However, I think it might be better to always show the notice, even if old env files don't exist. My travis CI broke after the cabal-head package was updated, and my doctests (which relied on the env files) suddenly stopped working. Since travis runs a clean build every time, the warning would not have been shown. If I got a warning about the env file not being generated, it would have been a little easier to get to the bottom of the problem.

Would always issuing a warning cause issues elsewhere?

@phadej
Copy link
Collaborator

phadej commented Apr 6, 2019 via email

@phadej
Copy link
Collaborator

phadej commented Apr 6, 2019 via email

@gbaz
Copy link
Collaborator

gbaz commented Apr 6, 2019

One other suggestion I would like to make -- I think we should have the "disable" setting in general control if the files are created in the project directory. However, we should probably either also unconditionally (or at least when files are not created in the project directory) create an appropriately named file (perhaps without a dot) in the dist-newstyle (or dist, i don't remember if we're renaming dist-newstyle to dist or not) directory, so that tools which start to make use of these files explicitly can know where to find one regardless of other settings.

Since part of the whole issue has been to make things as seamless as possible for various tooling, I think keeping that design consideration in mind going forward as well could be important.

@phadej phadej merged commit 233ae96 into haskell:master Apr 7, 2019
@phadej
Copy link
Collaborator

phadej commented Apr 7, 2019

I merged this. It's already an improvement, and following refinements can be done separately.

23Skidoo pushed a commit that referenced this pull request Apr 9, 2019
Disable .ghc-environment file generation by default
@23Skidoo
Copy link
Member

23Skidoo commented Apr 9, 2019

Also cherry-picked into 2.4. On reflection, I decided to revert the 2.4 cherry-pick since changing behaviour so drastically in minor releases is surprising and additional improvements in env file generation mentioned above haven't been implemented yet.

23Skidoo added a commit that referenced this pull request Apr 11, 2019
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.

None yet

8 participants