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

Add support for Cabal-syntax #254

Merged
merged 1 commit into from
Jan 22, 2022
Merged

Conversation

patrickdoc
Copy link
Contributor

Add support for new Cabal-syntax library: haskell/cabal#7620

The only reference to Cabal I didn't know how to remove was the CPP here: https://github.com/haskell/hackage-security/blob/master/hackage-security/tests/TestSuite.hs#L18

if flag(use-cabal-syntax)
build-depends: Cabal-syntax >= 3.7 && < 3.8
else
build-depends: Cabal >= 1.12
Copy link
Contributor

Choose a reason for hiding this comment

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

this should have also && <3.7 to make the flag selection options mutually exclusive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

else
build-depends: Cabal >= 1.14 && < 1.26
|| >= 2.0 && < 2.6
|| >= 3.0 && < 3.4
Copy link
Contributor

Choose a reason for hiding this comment

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

Excluding 3.6 on purpose?

Copy link
Contributor Author

@patrickdoc patrickdoc Oct 16, 2021

Choose a reason for hiding this comment

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

Not intentional, just naively cut-pasted from the existing constraints, updated now.

@phadej
Copy link
Contributor

phadej commented Oct 16, 2021

@Mikolaj would it make sense to commit recent revisions (https://hackage.haskell.org/package/hackage-security-0.6.0.1/revisions/) into the repository?

@Mikolaj
Copy link
Member

Mikolaj commented Oct 16, 2021

@Mikolaj would it make sense to commit recent revisions (https://hackage.haskell.org/package/hackage-security-0.6.0.1/revisions/) into the repository?

Absolutely. If nobody does it, I will do this before a release (probably early next week?).

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

As far as I understand the context, LGTM.

@@ -84,6 +85,13 @@ executable hackage-repo-tool
else
build-depends: network >= 2.5 && < 2.6

if flag(use-cabal-syntax)
build-depends: Cabal-syntax >= 3.7 && < 3.8
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I overlooked this: it's better to leave the bound <3.9 (here and elsewhere), so we don't need to do revisions right after the Cabal(-syntax)-3.8 is released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Member

Choose a reason for hiding this comment

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

@phadej: ping?

else
build-depends: Cabal >= 1.14 && < 1.26
|| >= 2.0 && < 2.6
|| >= 3.0 && < 3.7
Copy link
Contributor

Choose a reason for hiding this comment

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

this

else
build-depends: Cabal >= 1.14 && < 1.26
|| >= 2.0 && < 2.6
|| >= 3.0 && < 3.7
Copy link
Contributor

Choose a reason for hiding this comment

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

and this still have <3.7

Copy link
Contributor Author

@patrickdoc patrickdoc Oct 31, 2021

Choose a reason for hiding this comment

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

This logic is trying to handle the split, so for:

  • < 3.7, use Cabal
  • >= 3.7, use Cabal-syntax

Is that the wrong version to split on?

Copy link
Member

Choose a reason for hiding this comment

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

@phadej: ping? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, nevermind.

Copy link
Contributor

@phadej phadej Nov 3, 2021

Choose a reason for hiding this comment

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

Is that the wrong version to split on?

Not really. It's a bit unfortunate as the sections are not mutually exclusive. There could be install-plan with Cabal-syntax-3.7 and Cabal-3.5, so the flag value selection is arbitrary.

This could be problematic, e.g. something which uses old Cabal version still, then the solver will pick the Cabal-syntax-3.7 dependency for hackage-security. And then weird things might happen as e.g. download has PackageIdentifier in its type. Downstream would expect one from Cabal-3.5, but hackage-security would have Cabal-syntax-3.7 one.

A solution to this is to release a dummy Cabal-syntax-3.6 which would depend on Cabal <3.7 and use its version to get mutual exclusive branches. The similar example is https://hackage.haskell.org/package/network-uri-2.5.0.0 but I think it's wrong as network-uri-2.5.0.0 doesn't depend on network we have to write:

  if flag(network-uri)
    build-depends: network-uri >= 2.6, network >= 2.6
  else
    build-depends: network-uri < 2.6, network < 2.6

but we would like (in our case) to be able to omit the "network" from the newer branch:

  if flag(network-uri)
    build-depends: network-uri >= 2.6
  else
    build-depends: network-uri < 2.6, network < 2.6

I.e. we would write

 if flag(Cabal-syntax)
    build-depends: Cabal-syntax >= 3.7 && < 3.9
  else
    build-depends: Cabal-syntax <3.7, Cabal >= 1.12 ...

I'm very tired now, please check that I didn't any mistake in my reasoning.

Copy link
Contributor

Choose a reason for hiding this comment

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

... and I would rename the flag to just Cabal-syntax. It's common to just use the package name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand flags correctly, assuming we have some package foo that depends on Cabal-3.5 and the updated hackage-security, then the solver will return Cabal-3.5 and Cabal-syntax-3.7. And the maintainer of foo would need to work around that?

Perhaps it would be more user friendly to flip the default so that hackage-security will use Cabal < 3.7 and the dummy Cabal-syntax-3.6 unless newer versions are specifically requested (Cabal >= 3.7 or Cabal-syntax >= 3.7)?

In either case, what's the preferred path for generating the dummy cabal-syntax?

Copy link
Member

Choose a reason for hiding this comment

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

If nobody objects, I will help to set that in motion.

Copy link
Contributor

@phadej phadej left a comment

Choose a reason for hiding this comment

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

@Mikolaj don't ping me, if it looks good to you, go for it. The more I look at this the more small things I find even in this small patch.

@@ -39,6 +39,10 @@ flag use-old-time
description: Are we using @old-time@?
manual: False

flag use-cabal-syntax
description: Are we using Cabal-syntax?
manual: False
Copy link
Contributor

Choose a reason for hiding this comment

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

Please specify the default. I never remember which one is a default.

if flag(use-cabal-syntax)
build-depends: Cabal-syntax >= 3.7 && < 3.9
else
build-depends: Cabal >= 1.12 && < 3.7
Copy link
Contributor

Choose a reason for hiding this comment

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

I probably got confused because this section doesn't look like all others.

But this is just an example, so no need to change

else
build-depends: Cabal >= 1.14 && < 1.26
|| >= 2.0 && < 2.6
|| >= 3.0 && < 3.7
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, nevermind.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 3, 2021

I will wait for a fixed point or a cyclic alternation and then merge. :)

@Mikolaj
Copy link
Member

Mikolaj commented Nov 29, 2021

I guess we reached the fixed point. @patrickdoc: what would we start with? Could we spell out the plan so that @phadej may correct us?

@Mikolaj
Copy link
Member

Mikolaj commented Dec 29, 2021

The plan is that I merge soon and release new hackage-security. Bound should guarantee that it won't break 3.6.* nor any earlier cabals when it releases. Changes in cabal will follow.

#256 Edit: and I will commit recent revisions (https://hackage.haskell.org/package/hackage-security-0.6.0.1/revisions/) into the repository first.

Edit2: and, as @fendor pointed out, we probably need to release the dummy Cabal-syntax package first (it turns out not to be released) --- do we get this right or has the plan changed?

@Mikolaj
Copy link
Member

Mikolaj commented Dec 29, 2021

@patrickdoc: re "I update the hackage-security pr to fix the bounds in the flag" from Matrix/IRC, I thought they are already correct in the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants