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

Merged
merged 1 commit into from
Jan 17, 2025

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.

validate.sh Outdated Show resolved Hide resolved
@geekosaur
Copy link
Collaborator

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

@mpickering mpickering added the merge me Tell Mergify Bot to merge label Jan 13, 2025
@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Jan 13, 2025
@mpickering
Copy link
Collaborator Author

I have rebased and assigned the merge label.

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 15, 2025
…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
@mergify mergify bot merged commit 5ac2f87 into master Jan 17, 2025
53 checks passed
@mergify mergify bot deleted the wip/t10283 branch January 17, 2025 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period
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