Don't use hpack when cabal file is in the directory

[parsonsmatt]

Closes #507

This PR makes hpack force generation of a cabal file. hpack will bail out early if the version of hpack used to generate an existing cabal file is newer than itself. This results in a package that can't be built.

[sternenseemann]

I'm not in favor of this and also suggest either hpack should be disabled by the user in such cases or the updated cabal file checked in. If the --force is desired, this can easily be achieved via an override. We also could add an argument to haskellPackages.mkDerivation like hpackFlags to make this even easier.

[cdepillabout]

I'm not in favor of this and also suggest either hpack should be disabled by the user in such cases or the updated cabal file checked in.

What's you're reasoning for this? It seems to me like this is in general an improvement.

[sternenseemann]

If a newer hpack version was used for generating the cabal file, this implies that newer features not supported by our hpack version may have been used. Therefore this is a serious warning in my opinion, indicating that we may generate an incorrect cabal file.

[cdepillabout]

this implies that newer features not supported by our hpack version may have been used.

My guess is that this is not a big problem in practice.

I think we have these three different cases to deal with:

1. package.yaml was generated by a newer version of hpack, but Nix uses an older version of hpack. The package.yaml file doesn't use any features from the newer version of hpack, so generating the .cabal file works with the older version of hpack.

   This is specifically the case where --force would help. I'm guessing this case would happen way more often than the other cases.

2. package.yaml was generated by a newer version of hpack, but Nix uses an older version of hpack. The package.yaml file uses a new feature from the newer version of hpack.

   In this case, regardless of whether we used the --force flag or not, compilation still fails (since we can't generate a .cabal file). We are no worse here because of --force.

3. package.yaml was generated by a newer version of hpack, but Nix uses an older version of hpack. The package.yaml file uses a new feature from the newer version of hpack. The older version of hpack is still able to compile the package.yaml file though, but this produces a .cabal file that isn't able to be compiled correctly.

   From an end-user perspective, they see their package still not compiling regardless of whether or not --force is used. We are no worse here because of --force.

4. package.yaml was generated by a newer version of hpack, but Nix uses an older version of hpack. The package.yaml file uses a new feature from the newer version of hpack. The older version of hpack is still able to compile the file though. This produces a .cabal file that is able to be compiled correctly, but this causes weird errors at runtime.

   I think this is the only "dangerous" outcome of using --force. I imagine this is pretty unlikely to happen, though. I'd be pretty surprised if anyone could come up with a realistic example of a package.yaml file where this occurred.

---

I think case 1 is by far what is mostly likely to happen, so using --force by default seems like it would just make a smoother workflow for most users. My guess is that the likelihood this causes problems is pretty low.

[maralorn]

I'm not in favor of this and also suggest either hpack should be disabled by the user in such cases or the updated cabal file checked in.

I think forcing the user to check in a downgraded hpack file sounds like a weird solution.

I think what we actually want is this.

1. Expose an easy way to allow hpack --force.
2. Fail when the --force is needed.
3. Show an easy to understand error message that explains what is going on and how to fix your derivation.

But well, that’s not easy to achieve. I am not sure what bullet to bite here. But I actually think the probability of having a user loose 2h in figuring this out or just giving up is higher than the probability of us quietly compiling in a wrong way.

[parsonsmatt]

Another option is to just not run hpack if there's a .cabal file already. It's considered good practice to store generated cabal files. I ran into this problem because I checked the cabal file in to the repository.

Looks like the default option is to use hpack if there is a package.yaml file. Maybe the logic should be to only use hpack if there is only a package.yaml file, and trust that a checked-in .cabal file is intended to be the source of truth.

[sternenseemann]

Looks like the default option is to use hpack if there is a package.yaml file. Maybe the logic should be to only use hpack if there is only a package.yaml file, and trust that a checked-in .cabal file is intended to be the source of truth.

Yeah, that sounds like the most sensible way forward. The logic is a bit more complicated though since the cabal file's name changes with the package name (can there even be multiple ones in a single directory?).

[parsonsmatt]

cabal errors out if there are multiple .cabal files in a directory.

λ ~/Projects/persistent-discover/ cp persistent-discover.cabal persistent-lmao.cabal
λ ~/Projects/persistent-discover/ cabal build 
When using configuration(s) from /home/matt/Projects/persistent-discover/cabal.project, the following errors occurred:
The package directory '.' contains multiple .cabal files (which is not currently supported).

I've updated the PR to not run hpack if there is a cabal file present in the directory.

[cdepillabout]

I've tested that this works as advertised.

If no .cabal file exists, then package.yaml is used. If a .cabal file exists, then package.yaml is not used regardless of whether or not it exists.

@maralorn @sternenseemann any objection to merging this?

[maralorn]

LGTM. Thank you!

Since this changes user facing behavior I think this should probably have a changelog entry, because it could theoretically break projects with out-dated checked-in cabal files. But apparently this project has no changelog. So whatever, I guess?^^

[sternenseemann]

My plan was to release a bunch of non-breaking changes and fixes as a minor release (Milestone #1), we'd be able to backport. If we still want to do that, we should push this PR into Milestone #2 and hold off on merging for now — unless we want to maintain two branches in this repository

I think the minor release would be great because it would be backport-able and includes a fix to the annoying callCabal2nix error message problem. However, I currently can't do anything to accelerate this process since it is blocked on NixOS/nixpkgs#125980.

[sternenseemann]

Rebased on master and squashed the commits. Would be fine with merging later!

[pwm]

Just my 2c: while I think this is ultimately a good change I think it should have been marked as breaking in the changelog. Previously the workings of cabal2nix was independent from the existence of .cabal files if package.yaml was present whilst now it depends on it. This can break downstream systems where now, if cabal2nix sees a .cabal file it removes

libraryToolDepends = [ hpack ];
prePatch = "hpack";

from the generated derivation and then those derivations break in an environment where .cabal files are not present (eg. CI).

[sternenseemann]

Yeah, because of the severity of the change it is the first item in the changelog. FWIW the stuff marked as “API breaking change” is mostly not that severe a change, but requires adjustments from downstream users. I note them explicitly when creating the changelog because it is important to keep track of API changes for figuring out the new PVP-compliant version number.

But it's a good point, I'll try making a section “Changes the average user needs to care about” for the next release.

[pwm]

@sternenseemann first of all thank you for your amazing work on all of haskell-nix! My comment above is mostly for anyone else stumbling on this issue. Nevertheless I think flagging these as you say in the changelog would be good, if possible.