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

Change recommendation: include generated cabal files #5210

Closed
snoyberg opened this issue Mar 4, 2020 · 13 comments
Closed

Change recommendation: include generated cabal files #5210

snoyberg opened this issue Mar 4, 2020 · 13 comments

Comments

@snoyberg
Copy link
Contributor

@snoyberg snoyberg commented Mar 4, 2020

For details of the proposal, see the blog post: https://tech.fpcomplete.com/blog/storing-generated-cabal-files

If you are in favor of this change, please press thumbs up below. If you are opposed, please press thumbs down. Of course, feel free to include additional comments below.

@snoyberg snoyberg added this to the P0: Blocking release milestone Mar 4, 2020
@snoyberg snoyberg self-assigned this Mar 4, 2020
@gromakovsky
Copy link

@gromakovsky gromakovsky commented Mar 5, 2020

One problem with this approach that I encountered is that rebases become more painful. We have a repo with many packages, each one has package.yaml and we store .cabal files in the repo. If someone updates .cabal file(s) in master and you update something different in .cabal files in your branch by updating package.yaml and regenerating them (e. g. you both add a new dependency), you will get unnecessary conflicts because hpack adds a line with a hash.

It's not something critical, I just wanted to warn people about this inconvenience.

@cdornan
Copy link

@cdornan cdornan commented Mar 7, 2020

I have been getting feedback from non-stack users complaining about repos with no cabal files. From their perspective it is frustrating so I am delighted with this proposal even if it might be a little more inconvenient. N retrospect Cabal files form the basis of the package system so it is better to include them directly.

snoyberg added a commit to commercialhaskell/stack-templates that referenced this issue Mar 8, 2020
@snoyberg
Copy link
Contributor Author

@snoyberg snoyberg commented Mar 8, 2020

OK, I'm going to move ahead with this. Thanks for the responses all. Closing as the discussion is done, will be merged to master fairly soon.

@snoyberg snoyberg closed this as completed Mar 8, 2020
snoyberg added a commit to snoyberg/yaml that referenced this issue Mar 8, 2020
snoyberg added a commit to commercialhaskell/pantry that referenced this issue Mar 8, 2020
snoyberg added a commit that referenced this issue Mar 9, 2020
This ties in to the overall goal of #5210. Lock files should no longer
include packages without cabal files. We could either remove those from
freeze files, or get rid of freeze files entirely. Since freeze files
are already on their way out, may as well just cut them off entirely
now.
snoyberg added a commit that referenced this issue Mar 9, 2020
snoyberg added a commit that referenced this issue Mar 10, 2020
This ties in to the overall goal of #5210. Lock files should no longer
include packages without cabal files. We could either remove those from
freeze files, or get rid of freeze files entirely. Since freeze files
are already on their way out, may as well just cut them off entirely
now.
snoyberg added a commit that referenced this issue Mar 10, 2020
@simonmichael
Copy link
Contributor

@simonmichael simonmichael commented Apr 15, 2020

Agreeing with gromakovsky that this can very often cause conflicts and pain when merging/cherry-picking/switching branches/trying different stack/hpack versions. I think it's worth changing hpack to fix this - if you agree, consider helping sol/hpack#380.

@ulidtko
Copy link

@ulidtko ulidtko commented Jul 30, 2020

Hi! Let me just describe one failure mode collateral from this change. Hopefully, it'll make searching for it easier and speed up resolutions. No response required.

First, the error message:

2020-07-30 10:18:19.050984: [error] Aeson exception:
Error in $.packages[19].completed: failed to parse field 'packages': failed to parse field 'completed': Could not parse a UnresolvedPackageLocationImmutable from: Object (fromList [("size",Number 294200.0),("url",String "https://github.com/private-company/vendor-fork-redacted/archive/fcbaf261368b7f63708351fb1b04eb7f5f0793a3.tar.gz"),("name",String "vendor-fork-redacted"),("version",String "0.9.1"),("sha256",String "54974f6f9851376292cb6f081eef2cd907396899a04af5776da2d2cc45e80fc6"),("pantry-tree",Object (fromList [("size",Number 5039.0),("sha256",String "bc3f1c77c2797f0d0915f5ffff38d5e3cf8cdc613507fe1ace9463bc2492aafc")]))])

Now the cause. In my case, local-development Stack v2.3.1 has produced unintended change to stack.yaml.lock which wasn't understood by Stack v2.1.3 on CI:

@@ -154,9 +140,6 @@ packages:
 - completed:
     size: 294200
     url: https://github.com/private-company/vendor-fork-redacted/archive/fcbaf261368b7f63708351fb1b04eb7f5f0793a3.tar.gz
-    cabal-file:
-      size: 4079
-      sha256: a24d964776e360d0545500c8b28ec254aa323fdf28fa55692a282a25b6ac652c
     name: vendor-fork-redacted
     version: 0.9.1
     sha256: 54974f6f9851376292cb6f081eef2cd907396899a04af5776da2d2cc45e80fc6

Reverting this unintended change fixes the error (as well as updating Stack on CI).

The error reporting could perhaps be better; but this behavior change is well-documented on the ChangeLog. Hope that helps

minoki added a commit to minoki/haskell-floating-point that referenced this issue Dec 27, 2020
minoki added a commit to minoki/haskell-floating-point that referenced this issue Dec 27, 2020
mgajda added a commit to mgajda/less-arbitrary that referenced this issue Jan 12, 2021
ashutoshrishi added a commit to ashutoshrishi/hunspell-hs that referenced this issue Mar 23, 2021
eyeinsky added a commit to eyeinsky/old-fw that referenced this issue Apr 7, 2021
robertmassaioli added a commit to robertmassaioli/network-api-support that referenced this issue Apr 13, 2021
@robertmassaioli
Copy link

@robertmassaioli robertmassaioli commented Apr 13, 2021

@snoyberg I don't think that this proposal works withpvp-bounds yet. My "case-in-point" is the https://github.com/dzhus/snaplet-redis repository that has not commited the .cabal file AND uses pvp-bounds: upper. If I were to commit the generated cabal file for that repository then we would have a cabal file comitted that is completely different from the cabal file that gets published to Hackage. In terms of reproducability, I think that is a large issue, it means that the same source code, accessed different ways, could give you different results.

Ideally, the stack build command would perform the pvp-bounds logic in the generated cabal file so that we could commit an accurate cabal file. Ping @dzhus

@dzhus
Copy link
Contributor

@dzhus dzhus commented Apr 14, 2021

Ideally, the stack build command would perform the pvp-bounds logic in the generated cabal file so that we could commit an accurate cabal file

Yeah in the world where you commit .cabal files I'm not sure I see the utility of --pvp-bounds being used only for sdist, if (any) file is generated by tooling it should generated once per immutable and reproducible version of the project, not modified via a conveyor belt of commands.

On a side note, reading up on this very proposal (that I was not aware of):

people using build tools without hpack support are still out of luck with these repos.

In my view that's an issue of individual workflow, not projects or ecosystem as a whole. Just like using chains of git: dependencies is just a choice to organise one's workflow (where as I understand the issue of "generating .cabal-s for deps become prohibitively expensive" comes from), the utility of which is slowly fading as CI resources become cheaper. As such I hope that this remains a recommendation, as the choice to commit .cabal files or not doesn't affect whether a library can be pushed to Hackage / included in Stackage.

BardurArantsson added a commit to BardurArantsson/cqrs that referenced this issue Apr 17, 2021
unhammer added a commit to unhammer/word2vec-model.hs that referenced this issue Oct 7, 2021
@Javran
Copy link

@Javran Javran commented Oct 24, 2021

Should this issue be revisited? The decision was made allowing only 4 days and with only a handful of votes - I'm not convinced that this represents a majority of users.

The blogpost says more about implementation details that I'm relunctant to dive deep (my apologies if any of my concerns below has been addressed in the blogpost), but from user's point of view, why I have to suffer this deprecation notice which is not even my fault? If reproducibility is the concern, I don't see git+commit doing any worse than package with digest. If space is the concern, yes hpack is an extra dependency, but does this really matter when ~/.stack usually accumlates to two digit gigabytes?

In addition, the reason why I choose git+commit over package with digest is because the dependency is under development and resides under a different repo, for fast prototyping that requires modifying both main project and dependency library, it's counterproductive to go through this cabal sdist-then-update route as suggested alternative in the blogpost. Sure when it comes releases I'll make sure it ships with a cabal file, but I don't see the reason to check in a generated file for source version control.

A potential workaround I can think of for now is to have both main project and library reside under the same repo, this might not be applicable to all use cases, unfortunately.

@hasufell
Copy link
Contributor

@hasufell hasufell commented Oct 24, 2021

but I don't see the reason to check in a generated file for source version control

Because you're breaking your repository for every cabal user: https://cabal.readthedocs.io/en/latest/cabal-package.html?highlight=source-package-repository#source-repositories

This only works under cabal if your cabal file is checked into git. hpack is a third-party tool, not a community standard and certainly not part of the Cabal library, which specifies how all haskell packages are to be built.

@Javran
Copy link

@Javran Javran commented Oct 24, 2021

@Hasufel It is my choice that the repo is managed by Stack, not Cabal. If it's a release meant to be general Haskell package, sure I'll fix it. But I'm stressing that I'd avoid checking in a generated file unless absolutely necessary (I think "source" as "source of truth" rather than "source from the Cabal point of view") - I don't think being compatible with Cabal (again, for the repo) fits that category.

@ulidtko
Copy link

@ulidtko ulidtko commented Oct 24, 2021

@Javran I'm in the same boat with you in regards to committing generated files coming to be a must. That's seriously not cool. My diff reading time is limited — just as anyone else's.

However, let me express a friendly skepsis: reverting the change of Official Recommendation (with associated counter-deprecations? how would that even...) anytime soon — will spawn a mess an order of magnitude worse. Than either of the two original options, both bad in their own ways: violating DRY and fostering bloat VS breaking naive non-Stack usage of the package.¹

Reverting will spawn a mess, simply due to confusion at scale. One doesn't simply undo a deprecation which has only recently started. Just take a look at how often this issue (which is linked directly in Stack's deprecation message) has been referenced from within GitHub this year. People are still discovering this change. The Stack #5210 deprecation is still in progress.

Since everyone discovers the deprecation at their own pace, and on longish timescales (months, years) — it won't be wise for Stack to take a sudden 180° change of direction in the middle of it. Doesn't even matter how great the technical arguments are.


What I found to work "well enough" in practice. Add a GitHub Linguist annotation in .gitattributes:

*.cabal linguist-generated=true

This blog shows the effect; you get diffs which auto-hide on GitHub. Won't help with rebase conflicts however.


¹ I say "naive" because... what's there preventing Cabal users from calling hpack themselves too? hpack is package.yamlfoo.cabal transpiler in a nutshell; it's not a uniquely Stack thing. Plop a rule into your build system, add dependency, done.

@Javran
Copy link

@Javran Javran commented Oct 24, 2021

@ulidtko Thanks for the suggestion, although I check diffs mostly though editor plugins or CLIs, which GitHub Linguist feature won't solve. (EDIT: I've just checked that adding -diff to .gitattributes might be sufficient, which I'll give it a try) I just want to kick off some conversation on this issue now that we've lived with this change more than one year and seek to find some common grounds on it.

I fully understand that reverting such decisions will cause huge disruption one way or another. Would it be practical to settle with status quo (i.e. not removing hpack support in the future) and an option to supress this warning with the acknowledgment that repo won't be Cabal-compatible? Another path could be some hooking mechanism to call hpack after repo is checked out, or customizing Setup.hs to call hpack as you've mentioned (though I've never worked with Setup.hs before, but I'm happy to explore a bit and see what I would end up with).

@Javran
Copy link

@Javran Javran commented Oct 24, 2021

Quick update: I see looking into customizing Setup.hs will almost certainly open a can of worms but I'm happy for now living with some .gitattributes changes, combined with suggestions from @ulidtko:

*.cabal linguist-generated=true -diff

anka-213 added a commit to anka-213/haskell-wordnet that referenced this issue Jan 10, 2022
Importing libraries without cabal files are now depricated
See: commercialhaskell/stack#5210
bens added a commit to bens/hs-web3 that referenced this issue Feb 10, 2022
akru pushed a commit to airalab/hs-web3 that referenced this issue Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants