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

fix: make wrappers project part of workspace #143

Merged
merged 11 commits into from
Jan 16, 2024
Merged

Conversation

imor
Copy link
Contributor

@imor imor commented Sep 11, 2023

What kind of change does this PR introduce?

A change to how the project is organized.

What is the current behavior?

The wrappers project is not part of the workspace. This has a few drawbacks:

  1. IDEs like rust-analyzer or IntelliJ IDEA Rust plugin don't work if the project root is opened in them. For example, I can't go to definition across projects.
  2. There are separate target folders in each project. If two projects depend upon a single dependency, its artefacts will be copied to each target folder separately, increasing disc usage.
  3. Cargo commands like test, fmt, clippy etc. need to be run separately in each folder increasing dev friction.

What is the new behavior?

The wrappers project is part of the workspace. This fixes the above drawbacks and makes the project work with IDEs, with less disc usage and easier cargo usage during development and in GH actions.

Following changes were done as part of this:

  1. File wrappers/.cargo/config.toml was moved to .cargo/config.toml as mentioned in the pgrx docs about moving to a workspace.
  2. All three subprojects, wrappers, supabase-wrappers and supabase-wrappers-macros are part of the workspace in the top level Cargo.toml. In addition, the top level Cargo.toml contains the profiles now, fixing the profiles warning.
  3. Set "rust-analyzer.cargo.features": ["all_fdws,helloworld_fdw"] in .vscode/settings.json to make rust-analyzer in VSCode recognize all rust files in wrappers/src/fdw folder as part of the workspace.
  4. In order to avoid accidentally publishing wrappers to crates.io, a publish = false line is added to its Cargo.toml file.
  5. Update release workflow to look for build artefacts in workspace's target directory rather than in wrappers/target.

Additional Context

I also had to pin the clickhouse-rs to a specific commit in the Cargo.toml depdencies because of its accidental upgrade when adding wrappers to the workspace. Edit: clickhouse-rs dependency is already pinned to a specific revision in the main branch which is merged in this PR's branch.

To avoid accidentally publishing wrappers project, its Cargo.toml file now
contains `publish = false`
@imor
Copy link
Contributor Author

imor commented Sep 21, 2023

When member crates have profiles building the workspace emits a warning:

warning: profiles for the non root package will be ignored, specify profiles at the workspace root:

There's an open issue in cargo for this here. We need to find a way around this before this can be merged.

@imor imor requested review from burmecia and olirice January 15, 2024 12:26
@imor imor merged commit d431b9a into main Jan 16, 2024
2 checks passed
@imor imor deleted the fix/add-wrappers-to-workspace branch January 16, 2024 05:47
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