Skip to content

Sanitize delete-cache-on-linker-errors make target#2623

Merged
supersven merged 3 commits intodevelopfrom
sventennie/better-cache-deletion
Aug 22, 2022
Merged

Sanitize delete-cache-on-linker-errors make target#2623
supersven merged 3 commits intodevelopfrom
sventennie/better-cache-deletion

Conversation

@supersven
Copy link
Contributor

@supersven supersven commented Aug 17, 2022

dist-newstyle: The directory name depends on the cabal version. cabal clean does the job (using the tool itself to manage it's cache).

~/.cabal: We shouldn't delete the whole folder as it may contain a manually tweaked config file. Deleting ~/.cabal/store should be sufficient.

E.g. my ~/.cabal/config contains this to get docs for all packages in Haskell Language Server:

program-default-options
  ghc-options: -haddock

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • If this PR changes development workflow or dependencies, they have been A) automated and B) documented under docs/developer/. All efforts have been taken to minimize development setup breakage or slowdown for co-workers.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

`dist-newstyle`: The directory name depends on the cabal version. `cabal
clean` does the job (using the tool itself to manage it's cache).

`~/.cabal`: We shouldn't delete the whole folder as it may contain a
manually tweaked `config` file. Deleting `~/.cabal/store` should be
sufficient.
@supersven supersven temporarily deployed to cachix August 17, 2022 06:57 Inactive
@supersven supersven temporarily deployed to cachix August 17, 2022 06:57 Inactive
@supersven supersven requested review from jschaul and smatting August 17, 2022 06:57
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 17, 2022
@sysvinit
Copy link
Contributor

I have a soft nack against this make target, as I have my environment set up with the CABAL_DIR environment variable so that cabal stores its data somewhere other than ~/.cabal. However, I strongly suspect I'm a small minority here, so I won't block the PR, as it's easy enough for me to do a cache bust by hand when necessary.

@fisx
Copy link
Contributor

fisx commented Aug 17, 2022

I have a soft nack against this make target, as I have my environment set up with the CABAL_DIR environment variable so that cabal stores its data somewhere other than ~/.cabal. However, I strongly suspect I'm a small minority here, so I won't block the PR, as it's easy enough for me to do a cache bust by hand when necessary.

gotta protect minorities!! :)

also, isn't there a simple change that would honor $CABAL_DIR and do it right for all of us and for you?

@sysvinit
Copy link
Contributor

also, isn't there a simple change that would honor $CABAL_DIR and do it right for all of us and for you?

For sure -- I was mostly meaning to point out that the make target as written is incompatible with setting CABAL_DIR, but it doesn't need to be perfect here and now.

@supersven supersven temporarily deployed to cachix August 17, 2022 09:45 Inactive
@supersven supersven temporarily deployed to cachix August 17, 2022 09:45 Inactive
@supersven
Copy link
Contributor Author

Thanks for bringing this up, @sysvinit !

Would this work for you: 8101c5b ?

@smatting smatting temporarily deployed to cachix August 17, 2022 16:13 Inactive
@smatting smatting temporarily deployed to cachix August 17, 2022 16:13 Inactive
@smatting
Copy link
Contributor

Sorry for the merge commit @supersven . I used GH's "resolve conflicts" feature after merging #2621 - feel free to throw away and rebase

@supersven
Copy link
Contributor Author

Sorry for the merge commit @supersven . I used GH's "resolve conflicts" feature after merging #2621 - feel free to throw away and rebase

No worries, @smatting . I hope it will just vanish when I squash for merge.

@sysvinit
Copy link
Contributor

Thanks for bringing this up, @sysvinit !

Would this work for you: 8101c5b ?

Yeah, that looks good.

@supersven supersven merged commit 2de5ca1 into develop Aug 22, 2022
@supersven supersven deleted the sventennie/better-cache-deletion branch August 22, 2022 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants