Skip to content

fix: in workspace discovery consider the exclude list earlier on #3387

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

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

Conversation

ParkMyCar
Copy link
Contributor

In the Cargo.toml file for defining a Cargo Workspace, you can specify an exclude key which supports glob patterns to ignore entire subtrees.

This PR adds some logic to the directory walking loop to consider these patterns very early on. There is already some logic to handle these exclude paths, but it doesn't handle globs and the existing logic is after we try to read a Manifest, which is problematic if part of your file tree has a Cargo.toml that is invalid or difficult to read, e.g. part of a virtual environment.

@ParkMyCar ParkMyCar force-pushed the rendering/workspace-excludes branch from dd65df4 to 812456f Compare April 4, 2025 18:25
.transpose()?
.unwrap_or_default();

// TODO: It would be nice if WalkDir could skip entire directories so
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't you use filter_entry for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh perfect! I was only looking at the API on the WalkDir struct so missed that. Will update the PR!

ParkMyCar added a commit to MaterializeInc/materialize that referenced this pull request Apr 5, 2025
This PR upgrades our use of `rules_rust` to `v0.59.3`, which is a custom
version I released on our
[fork](https://github.com/MaterializeInc/rules_rust/releases/tag/mz-0.59.3).
As described on the release, it's a fork of the upstream `v0.59.2` and
includes two PRs I made upstream:

1. bazelbuild/rules_rust#3386
2. bazelbuild/rules_rust#3387

In addition to `rules_rust` we upgrade (out of necessity)

* `rules_cc` - rules for building C/C++ dependencies
* `protobuf` - required by the upgrade of `rules_cc`
* `bazel_skylib` - stdlib like Bazel rules

Note: This regresses our support for Apple x86_64 because I didn't have
a machine I could use to rebuild the `cargo-bazel` binary. But AFAIK
everyone using a Macbook now has an ARM based machine so this shouldn't
be an issue.

### Motivation

Unblocks our upgrade to Rust 1.86.0 and Edition 2024

### Checklist

- [ ] This PR has adequate test coverage / QA involvement has been duly
considered. ([trigger-ci for additional test/nightly
runs](https://trigger-ci.dev.materialize.com/))
- [ ] This PR has an associated up-to-date [design
doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md),
is a design doc
([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)),
or is sufficiently small to not require a design.
  <!-- Reference the design in the description. -->
- [ ] If this PR evolves [an existing `$T ⇔ Proto$T`
mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md)
(possibly in a backwards-incompatible way), then it is tagged with a
`T-proto` label.
- [ ] If this PR will require changes to cloud orchestration or tests,
there is a companion cloud PR to account for those changes that is
tagged with the release-blocker label
([example](MaterializeInc/cloud#5021)).
<!-- Ask in #team-cloud on Slack if you need help preparing the cloud
PR. -->
- [ ] If this PR includes major [user-facing behavior
changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note),
I have pinged the relevant PM to schedule a changelog post.
Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

I'm happy to do the filtering earlier, but I was under the impression that exclude didn't support globs. I'm pretty sure I tested this out when implementing this to check - can you verify it does support globs?

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.

3 participants