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

Hackage roundtrip tests are failing for package io-classes due to dependency splitting #10283

Open
fgaz opened this issue Aug 27, 2024 · 7 comments · May be fixed by #10287
Open

Hackage roundtrip tests are failing for package io-classes due to dependency splitting #10283

fgaz opened this issue Aug 27, 2024 · 7 comments · May be fixed by #10287
Assignees

Comments

@fgaz
Copy link
Member

fgaz commented Aug 27, 2024

jasagredo

This cabal file just broke the hackage roundtrip tests I think https://hackage.haskell.org/package/io-classes-1.6.0.0/io-classes.cabal

12345678910111213              +Dependency
                (PackageName "io-classes")
                (OrLaterVersion (mkVersion [0]))
                mainLibSet,
              Dependency
                (PackageName "io-classes")
                (OrLaterVersion (mkVersion [0]))
                (NonEmptySet.fromNonEmpty
                  (NE.fromList
                    [
                      -LMainLibName,
                      LSubLibName (UnqualComponentName "si-timers")]))],
            buildTools = ...},

Seems like parse . toUTF8BS . showGenericPackageDescription . parse changes the build-depends: io-classes:{io-classes, si-timers} to build-depends: io-classes, io-classes:si-timers
^ To reproduce: cabal run hackage-tests -- roundtrip io-classes

Originally posted by @mpickering in #10277 (comment)

@fgaz fgaz added the type: bug label Aug 27, 2024
@mpickering mpickering self-assigned this Aug 27, 2024
@mpickering
Copy link
Collaborator

The error is introduced by this function

257 -- See Note [Dependencies on sublibraries] in Distribution.PackageDescription.Parsec
258 --                                                                              
259 preProcessInternalDeps :: CabalSpecVersion -> GenericPackageDescription -> GenericPackageDescription
260 preProcessInternalDeps specVer gpd                                                    
261   | specVer >= CabalSpecV3_4 = gpd                                              
262   | otherwise = transformAllBuildInfos transformBI transformSBI gpd             
263   where                                                                         
264     transformBI :: BuildInfo -> BuildInfo                                                       
265     transformBI =                                                               
266       over L.targetBuildDepends (concatMap transformD)                          
267         . over L.mixins (map transformM)                                        
268                                                                                 
269     transformSBI :: SetupBuildInfo -> SetupBuildInfo                            
270     transformSBI = over L.setupDepends (concatMap transformD)                   
271                                                                                 
272     transformD :: Dependency -> [Dependency]                                    
273     transformD (Dependency pn vr ln)                                            
274       | pn == thisPn =                                                          
275           if LMainLibName `NES.member` ln                                       
276             then Dependency thisPn vr mainLibSet : sublibs                      
277             else sublibs                                                        
278       where                                                                     
279         sublibs =                                                               
280           [ Dependency (unqualComponentNameToPackageName uqn) vr mainLibSet       
281           | LSubLibName uqn <- NES.toList ln                                    
282           ]                                                                     
283     transformD d = [d]                                                          
284                                                                                 
285     transformM :: Mixin -> Mixin                                                
286     transformM (Mixin pn (LSubLibName uqn) inc)                                 
287       | pn == thisPn =                                                          
288           mkMixin (unqualComponentNameToPackageName uqn) LMainLibName inc       
289     transformM m = m                                                            
290                                                                                 
291     thisPn :: PackageName                                                       
292     thisPn = pkgName (package (packageDescription gpd)) 

@mpickering
Copy link
Collaborator

Setting the cabal-version to 3.4 resolves the roundtripping error (see first clause in preProcessInternalDeps).

@mpickering
Copy link
Collaborator

Linked to #6083

@mpickering
Copy link
Collaborator

Minimised reproducer:

cabal-version:       3.0
name:                io-classes
version:             1.6.0.0

library

library io-classes-mtl
    build-depends:  io-classes:{io-classes,si-timers}

@mpickering
Copy link
Collaborator

mpickering commented Aug 27, 2024

It seems a bit baffling to me that preProcessInternalDeps is called only before pretty printing. I think this code can probably be deleted. It seems that is used to be called in more places.

mpickering added a commit that referenced this issue Aug 27, 2024
…cit syntax

* In `postProcessInternalDeps` the shadowing logic which existed prior
  to cabal format 3.4 is implemented in a post processing step.

  The algorithm there replaces any references to internal sublibraries
  with an explicit qualifier. For example if you write..

  ```
  library
    build-depends: foo

  library foo
    ...
  ```

  this is reinterpreted as

  ```
  library
    build-depends: mylib:foo

  library foo
    ...
  ```

* In `preProcessInternalDeps` the inverse transformation takes place,
  the goal is to replace `mylib:foo` with just `foo`.

* Things go wrong if you are using version 3.0 for your cabal file
  because
  - In 3.0 the qualifier syntax is introduced so you can be expliciit
    about sublibrary dependencies
  - The shadowing semantics of non-qualified dependencies still exists.

  So the situation is that the user is explicit about the sublibrary

  ```
  library

  library qux
    build-depends: mylib:{mylib, foo}

  library foo
  ```

  1. Post-process leaves this alone, the user is already explicit about
     depending on a sublibrary.
  2. Pre-processing then rewrites `mylib:{mylib, foo}` into two
     dependencies, `mylib` and `foo` (no qualifier).
  3. When parsed these are two separate dependencies rather than treated
     as one dependency, roundtrip test fails.

Solution: Only perform the reverse transformation when the cabal library
version is <= 3.0 and doesn't support the explicit syntax.

Now what happens in these two situations:

1. ```
   library
     build-depends: foo

   library foo
     ...
   ```

  this is reinterpreted as

  ```
  library
    build-depends: mylib:foo

  library foo
    ...
  ```

  then printed and parsed exactly the same way.

2. Explicit syntax is parsed and printed without being munged (when
   supported)

Note: Mixins only supported sublibrary qualifiers from 3.4 whilst
dependencies supported this from 3.0, hence the lack of guard on the
mixins case.

Fixes #10283
mpickering added a commit that referenced this issue Aug 27, 2024
…cit syntax

* In `postProcessInternalDeps` the shadowing logic which existed prior
  to cabal format 3.4 is implemented in a post processing step.

  The algorithm there replaces any references to internal sublibraries
  with an explicit qualifier. For example if you write..

  ```
  library
    build-depends: foo

  library foo
    ...
  ```

  this is reinterpreted as

  ```
  library
    build-depends: mylib:foo

  library foo
    ...
  ```

* In `preProcessInternalDeps` the inverse transformation takes place,
  the goal is to replace `mylib:foo` with just `foo`.

* Things go wrong if you are using version 3.0 for your cabal file
  because
  - In 3.0 the qualifier syntax is introduced so you can be expliciit
    about sublibrary dependencies
  - The shadowing semantics of non-qualified dependencies still exists.

  So the situation is that the user is explicit about the sublibrary

  ```
  library

  library qux
    build-depends: mylib:{mylib, foo}

  library foo
  ```

  1. Post-process leaves this alone, the user is already explicit about
     depending on a sublibrary.
  2. Pre-processing then rewrites `mylib:{mylib, foo}` into two
     dependencies, `mylib` and `foo` (no qualifier).
  3. When parsed these are two separate dependencies rather than treated
     as one dependency, roundtrip test fails.

Solution: Only perform the reverse transformation when the cabal library
version is <= 3.0 and doesn't support the explicit syntax.

Now what happens in these two situations:

1. ```
   library
     build-depends: foo

   library foo
     ...
   ```

  this is reinterpreted as

  ```
  library
    build-depends: mylib:foo

  library foo
    ...
  ```

  then printed and parsed exactly the same way.

2. Explicit syntax is parsed and printed without being munged (when
   supported)

Note: Mixins only supported sublibrary qualifiers from 3.4 whilst
dependencies supported this from 3.0, hence the lack of guard on the
mixins case.

Fixes #10283
mpickering added a commit that referenced this issue Aug 27, 2024
…cit syntax

* In `postProcessInternalDeps` the shadowing logic which existed prior
  to cabal format 3.4 is implemented in a post processing step.

  The algorithm there replaces any references to internal sublibraries
  with an explicit qualifier. For example if you write..

  ```
  library
    build-depends: foo

  library foo
    ...
  ```

  this is reinterpreted as

  ```
  library
    build-depends: mylib:foo

  library foo
    ...
  ```

* In `preProcessInternalDeps` the inverse transformation takes place,
  the goal is to replace `mylib:foo` with just `foo`.

* Things go wrong if you are using version 3.0 for your cabal file
  because
  - In 3.0 the qualifier syntax is introduced so you can be expliciit
    about sublibrary dependencies
  - The shadowing semantics of non-qualified dependencies still exists.

  So the situation is that the user is explicit about the sublibrary

  ```
  library

  library qux
    build-depends: mylib:{mylib, foo}

  library foo
  ```

  1. Post-process leaves this alone, the user is already explicit about
     depending on a sublibrary.
  2. Pre-processing then rewrites `mylib:{mylib, foo}` into two
     dependencies, `mylib` and `foo` (no qualifier).
  3. When parsed these are two separate dependencies rather than treated
     as one dependency, roundtrip test fails.

Solution: Only perform the reverse transformation when the cabal library
version is <= 3.0 and doesn't support the explicit syntax.

Now what happens in these two situations:

1. ```
   library
     build-depends: foo

   library foo
     ...
   ```

  this is reinterpreted as

  ```
  library
    build-depends: mylib:foo

  library foo
    ...
  ```

  then printed and parsed exactly the same way.

2. Explicit syntax is parsed and printed without being munged (when
   supported)

Note: Mixins only supported sublibrary qualifiers from 3.4 whilst
dependencies supported this from 3.0, hence the lack of guard on the
mixins case.

Fixes #10283
@coot
Copy link
Collaborator

coot commented Aug 27, 2024

Do you want me to publish a revision which is using cabal-version: 3.4? Or maybe you don't want me to do that before you have a general fix?

@mpickering
Copy link
Collaborator

mpickering commented Aug 27, 2024

@coot At this point it doesn't matter what you do as the cabal file which exposes the bug is in the index tarball and will never be deleted. (Not a problem, it was a real bug which was exposed by the package)

mpickering added a commit that referenced this issue Aug 27, 2024
…cit syntax

* In `postProcessInternalDeps` the shadowing logic which existed prior
  to cabal format 3.4 is implemented in a post processing step.

  The algorithm there replaces any references to internal sublibraries
  with an explicit qualifier. For example if you write..

  ```
  library
    build-depends: foo

  library foo
    ...
  ```

  this is reinterpreted as

  ```
  library
    build-depends: mylib:foo

  library foo
    ...
  ```

* In `preProcessInternalDeps` the inverse transformation takes place,
  the goal is to replace `mylib:foo` with just `foo`.

* Things go wrong if you are using version 3.0 for your cabal file
  because
  - In 3.0 the qualifier syntax is introduced so you can be expliciit
    about sublibrary dependencies
  - The shadowing semantics of non-qualified dependencies still exists.

  So the situation is that the user is explicit about the sublibrary

  ```
  library

  library qux
    build-depends: mylib:{mylib, foo}

  library foo
  ```

  1. Post-process leaves this alone, the user is already explicit about
     depending on a sublibrary.
  2. Pre-processing then rewrites `mylib:{mylib, foo}` into two
     dependencies, `mylib` and `foo` (no qualifier).
  3. When parsed these are two separate dependencies rather than treated
     as one dependency, roundtrip test fails.

Solution: Only perform the reverse transformation when the cabal library
version is <= 3.0 and doesn't support the explicit syntax.

Now what happens in these two situations:

1. ```
   library
     build-depends: foo

   library foo
     ...
   ```

  this is reinterpreted as

  ```
  library
    build-depends: mylib:foo

  library foo
    ...
  ```

  then printed and parsed exactly the same way.

2. Explicit syntax is parsed and printed without being munged (when
   supported)

Note: Mixins only supported sublibrary qualifiers from 3.4 whilst
dependencies supported this from 3.0, hence the lack of guard on the
mixins case.

Fixes #10283
mpickering added a commit that referenced this issue Aug 28, 2024
…cit syntax

* In `postProcessInternalDeps` the shadowing logic which existed prior
  to cabal format 3.4 is implemented in a post processing step.

  The algorithm there replaces any references to internal sublibraries
  with an explicit qualifier. For example if you write..

  ```
  library
    build-depends: foo

  library foo
    ...
  ```

  this is reinterpreted as

  ```
  library
    build-depends: mylib:foo

  library foo
    ...
  ```

* In `preProcessInternalDeps` the inverse transformation takes place,
  the goal is to replace `mylib:foo` with just `foo`.

* Things go wrong if you are using version 3.0 for your cabal file
  because
  - In 3.0 the qualifier syntax is introduced so you can be expliciit
    about sublibrary dependencies
  - The shadowing semantics of non-qualified dependencies still exists.

  So the situation is that the user is explicit about the sublibrary

  ```
  library

  library qux
    build-depends: mylib:{mylib, foo}

  library foo
  ```

  1. Post-process leaves this alone, the user is already explicit about
     depending on a sublibrary.
  2. Pre-processing then rewrites `mylib:{mylib, foo}` into two
     dependencies, `mylib` and `foo` (no qualifier).
  3. When parsed these are two separate dependencies rather than treated
     as one dependency, roundtrip test fails.

Solution: Only perform the reverse transformation when the cabal library
version is <= 3.0 and doesn't support the explicit syntax.

Now what happens in these two situations:

1. ```
   library
     build-depends: foo

   library foo
     ...
   ```

  this is reinterpreted as

  ```
  library
    build-depends: mylib:foo

  library foo
    ...
  ```

  then printed and parsed exactly the same way.

2. Explicit syntax is parsed and printed without being munged (when
   supported)

Note: Mixins only supported sublibrary qualifiers from 3.4 whilst
dependencies supported this from 3.0, hence the lack of guard on the
mixins case.

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

Successfully merging a pull request may close this issue.

3 participants