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.

Sign up for GitHub

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

Jump to bottom

Look up sublibraries also with nonexact deps #8089

Merged
merged 4 commits into from
May 6, 2022
Merged

Look up sublibraries also with nonexact deps #8089

merged 4 commits into from
May 6, 2022

Conversation

fgaz
Copy link
Member

@fgaz fgaz commented Apr 8, 2022 •
edited
Loading

Fixes #7270 🎉
Closes #6801
Closes #7286

  • check that this does not break anything
  • and that there aren't other misuses of lookupDependency
    • maybe deprecate it?
  • remove the experimental warning
  • add a test

Would it be possible/worthwhile to do a 3.6 patch release with this, considering that this is blocking hackage upload for the "multiple public libraries" feature?

Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@fgaz fgaz mentioned this pull request Apr 8, 2022
Closed
@Mikolaj
Copy link
Member

Mikolaj commented Apr 8, 2022

There's no harm in a backport. About the actual release, perhaps let's wait and see if we need a release anyway, e.g., for patches that are necessary in GHC 9.4, and if we are able to pull off 3.8 or rather give up and release 3.6 again.

@fgaz
Copy link
Member Author

fgaz commented Apr 14, 2022 

/home/runner/.cabal/store/ghc-9.0.2/Cabal-3.6.3.0-5b25874bd4a212829928a04ead2f5bab253ba963aa995de42556b96bb18c9aec/lib/libHSCabal-3.6.3.0-5b25874bd4a212829928a04ead2f5bab253ba963aa995de42556b96bb18c9aec.a(Backpack.o)(.text+0x849c): error: undefined reference to 'base_GHCziRead_list3_info'

this is weird. maybe it's a caching problem?

@fgaz
Copy link
Member Author

fgaz commented Apr 15, 2022

Ok, that's definitely a caching problem, since the same pr in my fork succeeds. Travis had a useful "clear cache and rerun" button, but it looks like flushing the cache is impossible in github: actions/cache#2

@fgaz fgaz mentioned this pull request Apr 15, 2022
Closed
3 tasks
@jneira
Copy link
Member

jneira commented Apr 25, 2022

@Mergifyio rebase master

@mergify
Copy link
Contributor

mergify bot commented Apr 25, 2022

rebase master

✅ Branch has been successfully rebased

@fgaz fgaz force-pushed the fix-7270 branch 2 times, most recently from e905400 to 0cdca20 Compare May 2, 2022 10:20
fgaz added a commit to fgaz/cabal that referenced this pull request May 2, 2022
@fgaz
Changelog entry for haskell#8089
2fb3c2c
@fgaz fgaz mentioned this pull request May 3, 2022
Closed
@fgaz
Copy link
Member Author

fgaz commented May 3, 2022

The test that checks that experimental warnings are deduplicated relies on the warning I just removed... I could just remove that test or I could also remove PWTExperimental, since it isn't used anywhere else

@Mikolaj
Copy link
Member

Mikolaj commented May 4, 2022

The test that checks that experimental warnings are deduplicated relies on the warning I just removed... I could just remove that test or I could also remove PWTExperimental, since it isn't used anywhere else

IMHO, go for it.

fgaz added a commit to fgaz/cabal that referenced this pull request May 4, 2022
@fgaz
Changelog entry for haskell#8089
e0a1c04
fgaz added a commit to fgaz/cabal that referenced this pull request May 4, 2022
@fgaz
Changelog entry for haskell#8089
090ed11
fgaz added a commit to fgaz/cabal that referenced this pull request May 5, 2022
@fgaz
Changelog entries for haskell#8089
ee9d3db
@fgaz fgaz marked this pull request as ready for review May 5, 2022 06:59
fgaz added 4 commits May 5, 2022 09:00
@fgaz
Look up sublibraries also with nonexact deps
4817ff0 
When Cabal is passed --exact-configuration, it knows from
--dependency flags the ids of all dependencies, including sublibraries.
When this flag does not exists though, Cabal tries to locate them by
looking them up in the package databases. lookupDependency was used for
this, but it only looks up main (unnamed) libraries (LMainLibName), so
Cabal was failing to locate installed public sublibraries.
Using lookupInternalDependency and passing the library name fixes this.

Fixes haskell#7270
@fgaz
Add test for haskell#7270
6abd70a
@fgaz
Remove "colon specifier is experimental feature"
ee4a92d 
Same for "visibility is experimental feature"
With haskell#7270 fixed, it's time to allow hackage uploads.

PWTExperimental is not used anymore, but for now I'm keeping it in case
we add other experimental features in the future, so we don't keep
removing and adding it (pushing CPP on users).

Since it isn't used though there is no way to run
cabal-testsuite/PackageTests/DuplicateExperimental
so I'm just removing that test.

Closes haskell#6801
@fgaz
Changelog entries for haskell#8089
67e9a86
@fgaz
Copy link
Member Author

fgaz commented May 5, 2022

This is ready for review. I checked other lookupDependency uses and they are mostly fine, I'll leve the deprecation for another pr

Mikolaj
Mikolaj approved these changes May 5, 2022
View reviewed changes
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.

So the whole epic bug was not updating a single use of lookupDependency to lookupInternalDependency?

@fgaz
Copy link
Member Author

fgaz commented May 5, 2022

Yes 😔

@Mikolaj Mikolaj added this to the Considered for 3.8 milestone May 5, 2022
Kleidukos
Kleidukos approved these changes May 5, 2022
View reviewed changes
Copy link
Member

@Kleidukos Kleidukos left a comment

Choose a reason for hiding this comment

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

Nice fix ;)

@andreabedini
Copy link
Collaborator

Two approvals in. I am merging this one while everybody sleeps.

@andreabedini andreabedini merged commit da863c1 into haskell:master May 6, 2022
@gbaz gbaz mentioned this pull request Jun 12, 2022
Closed
@brprice brprice mentioned this pull request Aug 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kleidukos Kleidukos Kleidukos approved these changes

@Mikolaj Mikolaj Mikolaj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Considered for 3.8
6 participants
@fgaz @Mikolaj @jneira @andreabedini @Kleidukos @gbaz