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

Move tree-diff dependency to tests together with all instances #3893

Merged
merged 3 commits into from
Nov 29, 2023

Conversation

jasagredo
Copy link
Contributor

@jasagredo jasagredo commented Nov 27, 2023

Description

tree-diff uses GPL license so it should not be in the non-testing code in our case. This PR moves all the instances to the test packages so that only when testing this dependency is pulled in. See how when requesting to build just the libraries, no tree-diff is pulled from hackage:

cardano-ledger [ master][✘»!+?][λ 9.6.3]
❯ cabal clean

cardano-ledger [ master][✘»!+?][λ 9.6.3]
❯ rm -rf ~/.cabal/store/ghc-9.6.3/tree-diff-*

cardano-ledger [ master][✘»!+?][λ 9.6.3]
❯ cabal build cardano-ledger-shelley:lib:cardano-ledger-shelley cardano-ledger-byron:lib:cardano-ledger-byron cardano-ledger-allegra:lib:cardano-ledger-allegra cardano-ledger-mary:lib:cardano-ledger-mary cardano-ledger-alonzo:lib:cardano-ledger-alonzo cardano-ledger-conway:lib:cardano-ledger-conway cardano-ledger-babbage:lib:cardano-ledger-babbage cardano-ledger-api:lib:cardano-ledger-api set-algebra:lib:set-algebra cardano-crypto-wrapper:lib:cardano-crypto-wrapper cardano-data:lib:cardano-data cardano-ledger-binary:lib:cardano-ledger-binary vector-map:lib:vector-map small-steps:lib:small-steps non-integral:lib:non-integral --dry-run
Build profile: -w ghc-9.6.3 -O1
In order, the following would be built (use -v for more details):
 - non-integral-1.0.0.0 (lib) (first run)
 - small-steps-1.0.0.0 (lib) (first run)
 - vector-map-1.0.1.0 (lib) (first run)
 - cardano-ledger-binary-1.2.1.1 (lib) (first run)
 - cardano-data-1.1.2.0 (lib) (first run)
 - cardano-crypto-wrapper-1.5.1.0 (lib) (first run)
 - set-algebra-1.1.0.1 (lib) (first run)
 - cardano-ledger-byron-1.0.0.4 (lib) (first run)
 - cardano-ledger-core-1.10.0.0 (lib:internal) (first run)
 - cardano-ledger-core-1.10.0.0 (lib) (first run)
 - cardano-ledger-shelley-1.9.0.0 (lib:internal) (first run)
 - cardano-ledger-shelley-1.9.0.0 (lib) (first run)
 - cardano-ledger-allegra-1.2.5.1 (lib:internal) (first run)
 - cardano-ledger-allegra-1.2.5.1 (lib) (first run)
 - cardano-ledger-mary-1.4.0.1 (lib) (first run)
 - cardano-ledger-alonzo-1.6.0.0 (lib:internal) (first run)
 - cardano-ledger-alonzo-1.6.0.0 (lib) (first run)
 - cardano-ledger-babbage-1.6.0.0 (lib:internal) (first run)
 - cardano-ledger-babbage-1.6.0.0 (lib) (first run)
 - cardano-ledger-conway-1.12.0.0 (lib:internal) (first run)
 - cardano-ledger-conway-1.12.0.0 (lib) (first run)
 - cardano-ledger-api-1.8.0.0 (lib) (first run)

The PR is very verbose, but consists just of moving things around and exporting some internal constructors so that GHC can derive the instance in a separate module.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
    versioning process.
  • The version bounds in .cabal files for all affected packages are updated. If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
  • All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages. New section is never added with the code changes. (See RELEASING.md)
  • Code is formatted with fourmolu (use scripts/fourmolize.sh)
  • Cabal files are formatted (use scripts/cabal-format.sh)
  • hie.yaml has been updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

@jasagredo jasagredo requested a review from lehins November 27, 2023 10:16
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

I am in favor of moving the instances, however the way cabal files were adjusted is totally wrong

eras/allegra/impl/cardano-ledger-allegra.cabal Outdated Show resolved Hide resolved
@jasagredo jasagredo force-pushed the js/move-tree-diff-to-tests branch 5 times, most recently from 0de4870 to 10cdf38 Compare November 28, 2023 13:30
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

There was a bunch of unnecessary adjustments to bounds of dependencies, mostly for *-test packages, but other than that this PR is beautiful. Thank you for doing all this work!!!!

eras/conway/impl/testlib/Test/Cardano/Ledger/Conway/Imp.hs Outdated Show resolved Hide resolved
libs/cardano-ledger-api/CHANGELOG.md Show resolved Hide resolved
eras/shelley/test-suite/cardano-ledger-shelley-test.cabal Outdated Show resolved Hide resolved
eras/babbage/test-suite/cardano-ledger-babbage-test.cabal Outdated Show resolved Hide resolved
eras/babbage/impl/cardano-ledger-babbage.cabal Outdated Show resolved Hide resolved
eras/alonzo/test-suite/cardano-ledger-alonzo-test.cabal Outdated Show resolved Hide resolved
eras/alonzo/test-suite/cardano-ledger-alonzo-test.cabal Outdated Show resolved Hide resolved
@jasagredo jasagredo force-pushed the js/move-tree-diff-to-tests branch 2 times, most recently from bb315d7 to 65a71d4 Compare November 29, 2023 10:31
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Amazing stuff! Thank you for doing this! 👍

@jasagredo jasagredo force-pushed the js/move-tree-diff-to-tests branch from 65a71d4 to a98672a Compare November 29, 2023 12:01
@lehins lehins enabled auto-merge (squash) November 29, 2023 12:07
@lehins lehins merged commit 45c6789 into master Nov 29, 2023
9 of 17 checks passed
@iohk-bors iohk-bors bot deleted the js/move-tree-diff-to-tests branch November 29, 2023 13:59
lehins added a commit that referenced this pull request Dec 11, 2023
Fix a mistake introduced in #3893.
lehins added a commit that referenced this pull request Dec 11, 2023
Fix this mistake of exporting the `CostModel` cpnstructor that was introduced
in #3893.
lehins added a commit that referenced this pull request Dec 12, 2023
Fix this mistake of exporting the `CostModel` constructor, which was introduced
in #3893.
lehins added a commit that referenced this pull request Dec 12, 2023
Fix this mistake of exporting the `CostModel` constructor, which was introduced
in #3893.
lehins added a commit that referenced this pull request Dec 12, 2023
Fix this mistake of exporting the `CostModel` constructor, which was introduced
in #3893.
lehins added a commit that referenced this pull request Dec 12, 2023
Fix this mistake of exporting the `CostModel` constructor, which was introduced
in #3893.
lehins added a commit that referenced this pull request Dec 15, 2023
lehins added a commit that referenced this pull request Dec 15, 2023
Fix a bug introduced in #3893
lehins added a commit that referenced this pull request Dec 15, 2023
Fix a bug introduced in #3893
lehins added a commit that referenced this pull request Dec 15, 2023
Fix a bug introduced in #3893
lehins added a commit that referenced this pull request Dec 15, 2023
Fix a bug introduced in #3893
@lehins lehins mentioned this pull request Dec 15, 2023
9 tasks
lehins added a commit that referenced this pull request Dec 17, 2023
lehins added a commit that referenced this pull request Dec 17, 2023
Fix a bug introduced in #3893
lehins added a commit that referenced this pull request Dec 17, 2023
Fix a bug introduced in #3893
lehins added a commit that referenced this pull request Dec 17, 2023
lehins added a commit that referenced this pull request Dec 17, 2023
Fix a bug introduced in #3893
lehins added a commit that referenced this pull request Dec 17, 2023
Fix a bug introduced in #3893
lehins added a commit that referenced this pull request Dec 18, 2023
lehins added a commit that referenced this pull request Dec 18, 2023
Fix a bug introduced in #3893
lehins added a commit that referenced this pull request Dec 18, 2023
Fix a bug introduced in #3893
lehins added a commit that referenced this pull request Dec 18, 2023
lehins added a commit that referenced this pull request Dec 18, 2023
Fix a bug introduced in #3893
lehins added a commit that referenced this pull request Dec 18, 2023
Fix a bug introduced in #3893
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.

2 participants