Skip to content
This repository has been archived by the owner on Jun 17, 2024. It is now read-only.

feat: add dist-workspace.toml and factor some code #136

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Jun 7, 2024

This is a more fleshed out version of #124 that fills in all the relevant data more completely.

It also defines the workspace toml to be dist-workspace.toml, something we've frequently discussed before. However if there's no parent dist-workspace.toml, a dist.toml will be accepted as a workspace-with-only-one-package.

The repository URL logic has been hoisted out of rust because well, it should be generic, and also because we've learned the hard way that it just can't be this early bound -- cargo-dist has richer info on which packages we care about, and only once it has computed that should we be pedantic about repository url.

What this PR doesn't yet do is unify things such that generic and rust workspaces can be handled as a single heterogeneous entity. I'm going to do some initial upstream work in cargo-dist to feel that out more and avoid cooking my brain.

Copy link
Contributor

@mistydemeo mistydemeo left a comment

Choose a reason for hiding this comment

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

What's the usecase for calling repository_url with a specified list of packages instead of letting it pull from the internal info? Is the idea that cargo-dist will ask it about a subset of packages?

In testing, calling ws.repository_url(None) runs into an endless loop while calling ws.repository_url(Some(ws.packages().map...)) works.

@Gankra
Copy link
Contributor Author

Gankra commented Jun 10, 2024

What's the usecase for calling repository_url with a specified list of packages instead of letting it pull from the internal info? Is the idea that cargo-dist will ask it about a subset of packages?

Yeah specifically we want to only use the packages that cargo-dist selects (used in the downstream PR). Right now it's really annoying that we'll find a random repository key on a package we don't even care about and bail on the repository key altogether. axoproject is "too early" to make that call, so deferring it to later works better.

@Gankra Gankra marked this pull request as ready for review June 12, 2024 22:02
Copy link
Contributor

@mistydemeo mistydemeo left a comment

Choose a reason for hiding this comment

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

Tests look good, but can we make sure repository_url() gets tested?

src/tests.rs Outdated Show resolved Hide resolved
@Gankra
Copy link
Contributor Author

Gankra commented Jun 13, 2024

Ah I see, you're focusing on the build working at all -- right makes sense. I still think we should keep this around because nothing encourages buildjet and github to stay in sync, so forcing them in sync seems valuable.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants