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

crate_universe: Enable modext isolation. #17088

Merged
merged 3 commits into from
Aug 19, 2024
Merged

Conversation

criemen
Copy link
Collaborator

@criemen criemen commented Jul 29, 2024

This should allow us to build our python and ruby
code independently - in particular, we can now do shallow checkouts of one without the other.
Previously, the modext introduced a cross-dependency. This also reduces the amount of work we do in the
crate universe processing for the other language, even though it's unused.

This also gives us a speed bump of about 4min on all python-only jobs, as the main slowness of rules_rust is due to our ruby git dependencies, which we now don't pull in any longer.

The new file paths look like this:

rules_rust~~_crate~ql~~r~rd__tree-sitter-embedded-template-0.21.0
rules_rust~~_crate~ql~~cp~py_deps__tree-sitter-graph-0.7.0

@criemen criemen marked this pull request as ready for review July 29, 2024 07:48
@criemen criemen requested a review from a team as a code owner July 29, 2024 07:48
@criemen criemen marked this pull request as draft July 29, 2024 14:14
@criemen
Copy link
Collaborator Author

criemen commented Jul 29, 2024

Marking as draft, as this PR requires renaming the codeql module to ql, and shortening cargo_ruby to c, and ruby_deps to rd, to yield the same path lengths. Anything longer than that triggers long path errors with cl.exe on Windows 😬

I still think this is worth it, but this requires a bit more discussion than just enabling modext isolation.
The reason isolation requires longer names is that instead of crate, it uses _crate, and it adds the module name to the directory - that is duplicated twice, and so any more characters than the existing name blows up.

@criemen criemen force-pushed the criemen/modext-isolation branch from 6191d86 to cdbb036 Compare August 12, 2024 09:22
@github-actions github-actions bot added the Ruby label Aug 12, 2024
@criemen criemen force-pushed the criemen/modext-isolation branch from cdbb036 to 798efde Compare August 12, 2024 09:33
@criemen criemen force-pushed the criemen/modext-isolation branch 2 times, most recently from 388dc2c to a72c526 Compare August 12, 2024 10:00
@criemen criemen added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Aug 12, 2024
@criemen criemen marked this pull request as ready for review August 12, 2024 10:15
@criemen criemen requested review from a team as code owners August 12, 2024 10:15
load(
"@codeql//java/kotlin-extractor:versions.bzl",
"@ql//java/kotlin-extractor:versions.bzl",
Copy link
Contributor

Choose a reason for hiding this comment

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

There was some @codeqls in the comments at the start of this file; presumably they need to be changed too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I changed the relevant ones - when you build from the internal repo, we have an alias in place that you can still call the build targets @codeql//something, only from the external repo it's now (unfortunately) @ql//something, which is why I changed the wording of the comment there to remove the repo name alltogether when building from the external repo.
That said, there might still be an incorrect comment there, so if you find one, please shout!

MODULE.bazel Outdated
Comment on lines 1 to 5
module(
name = "codeql",
name = "ql",
version = "0.0",
)
Copy link
Contributor

@redsun82 redsun82 Aug 14, 2024

Choose a reason for hiding this comment

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

I think we can avoid having to change @codeql to @ql by using repo_name here as well:

Suggested change
module(
name = "codeql",
name = "ql",
version = "0.0",
)
module(
name = "ql",
version = "0.0",
repo_name = "codeql",
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is an excellent and unexpected find, thanks! This indeed makes things much nicer.

This should allow us to build our python and ruby
code independently - in particular, we can now do shallow
checkouts of one without the other.
Previously, the modext introduced cross-dependency.
This also reduces the amount of work we do in the
crate universe processing for the other language, even
though it's unused.

This does need renaming the module, as otherwise
the generated paths from rules_rust get too long
for Windows :(
@criemen criemen force-pushed the criemen/modext-isolation branch from a72c526 to f9bc97b Compare August 18, 2024 19:00
@github-actions github-actions bot removed the Kotlin label Aug 18, 2024
Copy link
Contributor

@redsun82 redsun82 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 good with these changes, thanks! I'd prefer to keep ruby_deps as an alias of rd though, as you can do that with use_repo (see the kwargs parameter in the docs).

@@ -30,27 +31,37 @@ bazel_dep(name = "rules_rust", version = "0.49.1")

bazel_dep(name = "buildifier_prebuilt", version = "6.4.0", dev_dependency = True)

crate = use_extension(
# crate_py but shortened due to Windows file path considerations
cp = use_extension(
Copy link
Contributor

Choose a reason for hiding this comment

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

really minor cosmetic nit, but is there any reason to use cp here and just r for ruby? If cr for ruby is already too much, maybe then also use p here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I wanted to limit the use of single-letter repo names to where we really need them (as there's only 26 of them available). The more letters, the easier to search for the repo name in our source code, too.

MODULE.bazel Outdated
cargo_lockfile = "//ruby/extractor:Cargo.lock",
manifests = [
"//ruby/extractor:Cargo.toml",
"//ruby/extractor/codeql-extractor-fake-crate:Cargo.toml",
],
)
use_repo(crate, "py_deps", "ruby_deps")
use_repo(r, "rd")
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also assign an alias here, avoiding the changes in the two build files:

Suggested change
use_repo(r, "rd")
use_repo(r, ruby_deps="rd")

@github-actions github-actions bot removed the Ruby label Aug 19, 2024
@criemen criemen requested a review from redsun82 August 19, 2024 06:56
@criemen criemen merged commit 2933a3b into main Aug 19, 2024
19 checks passed
@criemen criemen deleted the criemen/modext-isolation branch August 19, 2024 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depends on internal PR This PR should only be merged in sync with an internal Semmle PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants