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

Example repo #89

Merged
merged 4 commits into from
Jul 25, 2023
Merged

Example repo #89

merged 4 commits into from
Jul 25, 2023

Conversation

eed3si9n
Copy link
Contributor

This is an offshoot from #13

Problem

In the current state bzl-gen-build is not really usable outside of our own usage since it's relying on custom scripts and macros. This is often necessitated by our tendency to use old version of Python etc.

Solution

This adds an example repository to reimplement some of the scripts and configurations using open source rules_python etc.

Problem
-------
In the current state bzl-gen-build is not really usable
outside of our own usage since it's relying on custom scripts and macros.
This is often necessitated by our tendency to use old version of Python etc.

Solution
--------
This adds an example repository to reimplement some of the
scripts and configurations using open source rules_python etc.
@@ -123,12 +123,16 @@ async fn main() -> Result<()> {
}

fn expand_path_to_defs_from_offset(from_given_path: &str, path: &str) -> Vec<String> {
if let Some(rem) = path.strip_prefix(from_given_path) {
if let Some(rem0) = path.strip_prefix(from_given_path) {
let rem = match rem0.strip_prefix("site-packages/") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a minor change on Python extractor, needed because rules_python Bzlmod support uses pip-tools, which I think places the 3rdparty source files inside a site-packages/ directory, per module.

@@ -123,12 +123,16 @@ async fn main() -> Result<()> {
}

fn expand_path_to_defs_from_offset(from_given_path: &str, path: &str) -> Vec<String> {
if let Some(rem) = path.strip_prefix(from_given_path) {
if let Some(rem0) = path.strip_prefix(from_given_path) {
let rem = match rem0.strip_prefix("site-packages/") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if let Some(rem) = path.strip_prefix(from_given_path).and_then(|p| p.strip_prefix("site-packages/").unwrap_or(p) {

inner block is ok, too but i'd use unwrap_or here. Probably also just put your comment here into the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 456bc96

Copy link
Collaborator

@ianoc ianoc left a comment

Choose a reason for hiding this comment

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

one nit, otherwise lgtm

@@ -0,0 +1,2 @@
filegroup(name='hello_files', srcs=glob(include=['**/*.py']), visibility=['//visibility:public'])
py_library(name='hello', srcs=[':hello_files'], deps=['@@rules_python~0.24.0~pip~pip_39_pandas//:pkg'], visibility=['//visibility:public'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

that double @ looks fishy to me... is that actually correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea Bazel 6's module system seems to use @@:

$ bazel query @@rules_python~0.24.0~pip~pip_39_pytz//:pkg
@@rules_python~0.24.0~pip~pip_39_pytz//:pkg

@@ -0,0 +1,8 @@
java_test(
name = "Test1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is manually written right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I haven't hooked up Java support in this PR.

set -o pipefail # don't hide errors within pipes

cd example
# TOOLING_WORKING_DIRECTORY=/tmp/bzl-gen-build source build_tools/lang_support/create_lang_build_files/regenerate_protos_build_files.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not the protos and the java here btw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This example/ used to be part of the Protobuf PR, but it got too big, so I am splitting it up. I figured Python would be most self-contained (without jar scanner etc) to demonstrate, but eventually we should try to hook it up as well using the new jar scanner.


cd example
# TOOLING_WORKING_DIRECTORY=/tmp/bzl-gen-build source build_tools/lang_support/create_lang_build_files/regenerate_protos_build_files.sh
TOOLING_WORKING_DIRECTORY=/tmp/bzl-gen-build source build_tools/lang_support/create_lang_build_files/regenerate_python_build_files.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we add a test that the git diff in examples is a clean diff? Seems like it would be a good idea to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in cdec70c

@eed3si9n eed3si9n merged commit fbcc169 into bazeltools:main Jul 25, 2023
6 checks passed
@eed3si9n eed3si9n deleted the wip/example_repo branch July 25, 2023 21:08
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