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

Allow to publish packages with test dependencies not in the registry #1270

Merged
merged 16 commits into from
Aug 28, 2024

Conversation

fsoikin
Copy link
Contributor

@fsoikin fsoikin commented Aug 19, 2024

Description of the change

The primary motivation for this is allowing circular dependencies between packages for testing only. In this scenario one of the packages is added to extra_packages section of the other with a patched dependencies property. For example:

# github/collegevine/purescript-elmish/spago.yaml
workspace:
  extra_packages:
    elmish-testing-library:
      repo: https://github.com/collegevine/purescript-elmish-testing-library.git
      version: v2.0.0
      dependencies: [] # <-- no Elmish in dependencies, avoiding the cycle

package:
  name: purescript-elmish
  dependencies: ...
  test:
    main: ...
    dependencies: ["elmish-testing-library"]

This scenario was actually working fine for the most part, and the only obstacle was publishing the package. Because the extra_packages dependency is not in the package registry, Spago refuses to publish. This change makes it so that Spago will only apply this rule to "core" dependencies, but not to "test" ones. See also this Discord discussion.

To achieve this, the following changes have been made:

  • Fetch.run no longer returns a single list of dependencies for every package, but instead returns them in two buckets - "core" and "test".
  • The lockfile format has been changed to support the same structure:
    • from: { dependencies, build_plan, test_dependencies }
    • to: { core: { dependencies, build_plan }, test: { dependencies, build_plan } }
  • Most call sites of Fetch.run ultimately call toAllDependencies, which flattens the whole tree and combines core with test.
  • Publish.run specifically picks only core and drops test, so as not to perform all the rigorous checks for the test dependencies.
  • Build.getBuildGlobs no longer adds test globs (i.e. test/**/*.purs) for all workspace packages, but only for the "selected" ones. This is because only "selected" packages will have their test dependencies added to the build plan.

Checklist:

  • Added the change to the "Unreleased" section of the changelog
  • [ ] Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)

@fsoikin fsoikin force-pushed the publish-test-deps branch 2 times, most recently from c468e84 to 91e05be Compare August 21, 2024 16:03
spago.lock Show resolved Hide resolved
src/Spago/Command/Build.purs Show resolved Hide resolved
src/Spago/Command/Fetch.purs Show resolved Hide resolved
src/Spago/Command/Fetch.purs Show resolved Hide resolved
src/Spago/Command/Fetch.purs Outdated Show resolved Hide resolved
src/Spago/Command/Fetch.purs Show resolved Hide resolved
test-fixtures/publish-extra-package-core-dependency.txt Outdated Show resolved Hide resolved
@fsoikin fsoikin force-pushed the publish-test-deps branch 2 times, most recently from bf8a0c9 to a1657e5 Compare August 23, 2024 15:18
Copy link
Member

@f-f f-f left a comment

Choose a reason for hiding this comment

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

Brilliant refactoring of the new caching code in Fetch - @finnhodgkin could you test this code to see if the perf still holds up?

src/Spago/Command/Fetch.purs Outdated Show resolved Hide resolved
Copy link
Member

@f-f f-f left a comment

Choose a reason for hiding this comment

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

Stellar work! Thanks 👏

@f-f f-f merged commit 16502a1 into purescript:master Aug 28, 2024
5 checks passed
@fsoikin fsoikin deleted the publish-test-deps branch August 28, 2024 15:41
@fsoikin
Copy link
Contributor Author

fsoikin commented Aug 28, 2024

Awesome! Could you cut a new version soon so I can use it to publish spec?

@f-f
Copy link
Member

f-f commented Aug 28, 2024

Yeah I will merge #1275 now then cut a new version!

@f-f
Copy link
Member

f-f commented Aug 28, 2024

@fsoikin 0.93.39 is out 🚀

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