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

Only munge internal dependencies when printing when there is no explicit syntax #10287

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mpickering
Copy link
Collaborator

@mpickering mpickering commented Aug 27, 2024

Only munge internal dependencies when printing when there is no explicit 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

Please read Github PR Conventions and then fill in one of these two templates.


Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

@jasagredo
Copy link
Collaborator

I updated your PR description a bit to properly format the code blocks, I was not understanding what you were describing because lines were missing in the rendered markdown.

…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
@@ -419,7 +419,7 @@ CMD="$($CABALLISTBIN Cabal-tests:test:no-thunks-test) $TESTSUITEJOBS --hide-succ


# See #10284 for why this value is pinned.
HACKAGE_TESTS_INDEX_STATE="--index-state=2024-08-25"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The PR is tested by this change, as this hackage index has the failing .cabal file which failed to roundtrip.

@@ -282,9 +282,12 @@ preProcessInternalDeps specVer gpd
]
transformD d = [d]

-- Always perform transformation for mixins as syntax was only introduced in 3.4
-- This guard is uncessary as no transformations take place when cabalSpec < CabalSpecV3_4 but
Copy link
Member

Choose a reason for hiding this comment

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

I think you meant to use >=

@geekosaur
Copy link
Collaborator

#10285 (comment) re the merge conflict: validate.sh is a stub now.

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.

Hackage roundtrip tests are failing for package io-classes due to dependency splitting
5 participants