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

Detect when user changed .cabal file instead of package.yaml #3383

Closed
mgsloan opened this Issue Aug 23, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@mgsloan
Collaborator

mgsloan commented Aug 23, 2017

Use of package.yaml can be confusing when the user doesn't expect it - see #3380 . I think this case should somehow be detected and a noisy warning emitted.

One way to do this might be to output a copy of the *.cabal on hpack generation. Then, when later re-generating, check if the cabal file changed since the last hpack generation. If so, then the user modified it and a noisy warning should be displayed.

@borsboom

This comment has been minimized.

Contributor

borsboom commented Nov 1, 2017

A nice way would be to write a checksum into the .cabal file itself (of everything after the header comment), and then compare it before overwriting. That way don't need to keep track of two files (especially if the .cabal file is checked into git in addition to the package.yaml).

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Nov 1, 2017

Here's the logic I'd recommend:

  1. If package.yaml is newer than foo.cabal, ignore foo.cabal and overwrite it
  2. If foo.cabal is newer, check if the first line is of the form -- AUTOGEN CHECKSUM: sha256
  3. If the checksum does not exist:
    1. Generate a new copy of foo.cabal in memory
    2. If the new copy matches the existing one perfectly, write a new foo.cabal with the added CHECKSUM line
    3. If it does not match, error out with an explanation to the user that they need to compare the two files and then delete foo.cabal
  4. If the checksum exists and matches, all is well
  5. If the checksum exists and does not match, error out with an explanation to the user

Does this seem reasonable to everyone?

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Nov 9, 2017

@snoyberg Makes sense! I think that this should be implemented in hpack itself.

Marking this as P0 so that commercialhaskell/stack-templates#112 can be done sooner. If this ends up blocking release for too long, we can go ahead and ignore it. It might take a little while to add it to hpack and do a release of hpack. I will hopefully get around to it in the next few days, I have it on my TODO list.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Nov 9, 2017

I hadn't thought about this being in hpack instead. That makes sense, but also makes it pretty difficult to list as a P0 issue, since we have no control over the hpack release schedule.

In any event: CCing @sol. This issue is about the fact that a number of users have been tripped up by editing the generated cabal files instead of package.yaml files. We've put together a proposed solution above. How do you feel about including something like this in hpack?

@sol

This comment has been minimized.

Contributor

sol commented Nov 10, 2017

@snoyberg sounds sensible. I'm looking into it.

@sol

This comment has been minimized.

Contributor

sol commented Nov 10, 2017

As you guys want this as soon as possible, I implemented this based on the 0.19.3 release, so that we can do a minor release that just adds this feature.

I did some refactoring first, the actual change in behavior is in
sol/hpack@fc51e3e.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Nov 12, 2017

That's much appreciated, thanks @sol! I'll try to look at this today and provide some feedback.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Nov 12, 2017

This looks perfect, exactly what we were hoping for here. Thanks so much for doing this! What would you like from the Stack side before you're ready to make a release?

@sol

This comment has been minimized.

Contributor

sol commented Nov 15, 2017

@snoyberg I answered on sol/hpack#215.

snoyberg added a commit that referenced this issue Nov 16, 2017

snoyberg added a commit that referenced this issue Nov 16, 2017

snoyberg added a commit that referenced this issue Nov 19, 2017

@snoyberg snoyberg closed this in 57af148 Nov 19, 2017

snoyberg added a commit that referenced this issue Nov 19, 2017

Merge pull request #3579 from commercialhaskell/3383-detect-hpack-cab…
…al-changes

Upgrade to hpack 0.20.0 (fixes #3383)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment