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

feat: Enable header generation #35

Merged
merged 2 commits into from
May 22, 2024
Merged

Conversation

honnix
Copy link
Contributor

@honnix honnix commented May 21, 2024

Use --custom-compile-command together with label to enable header generation.

This will help users understand what bazel command to run in order to regenerate.

--python-version=$PYTHON_VERSION \
$(echo $PYTHON_PLATFORM) \
-o $REQUIREMENTS_TXT \
$REQUIREMENTS_IN \
$@
"$@"
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 bug fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, fair, we played a little fast-and-loose with the quoting here.

Copy link
Contributor

Choose a reason for hiding this comment

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

mind updating line 38 with the command to run to fix the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure let me try to understand why it failed. Will come to this shortly.

thm-automation bot pushed a commit that referenced this pull request May 21, 2024
## Issue
In #25 and #34 we've been trying to make it so that reviewbot doesn't fail when submissions come from 3rd party repos. #34 moved the suppression from the job to the step, but we still see a failure in #35 -- reading the failure more closely it seems to be because the secret is absent and the action validator still runs.

## Summary
Add an alternate value to the reviewbot secret to help pass validation and move the suppression back to the job.
@mark-thm
Copy link
Contributor

You can safely ignore the reviewbot/autosquash failures -- our automation doesn't work well with contributions from forks and we're apparently still not suppressing the runs well enough.

There is a legit CI failure in there, though.

@honnix
Copy link
Contributor Author

honnix commented May 21, 2024

You can safely ignore the reviewbot/autosquash failures -- our automation doesn't work well with contributions from forks and we're apparently still not suppressing the runs well enough.

There is a legit CI failure in there, though.

Hmm, this is a bit tricky:

-#    bazel run @@//:generate_requirements_linux_txt
+#    bazel run @@//:generate_requirements_linux_txt_diff_test

They are different because the labels are different.

@honnix
Copy link
Contributor Author

honnix commented May 21, 2024

It's not ideal, but 7c1a469 should fix the tests.

@honnix honnix marked this pull request as ready for review May 21, 2024 21:34
@honnix honnix requested a review from a team as a code owner May 21, 2024 21:34
@honnix
Copy link
Contributor Author

honnix commented May 21, 2024

Alternatively we could do

diff --git a/uv/private/pip.bzl b/uv/private/pip.bzl
index 9699fd7..00797df 100644
--- a/uv/private/pip.bzl
+++ b/uv/private/pip.bzl
@@ -1,6 +1,7 @@
 "uv based pip compile rules"

 _PY_TOOLCHAIN = "@bazel_tools//tools/python:toolchain_type"
+_DIFF_TEST_SUFFIX = "_diff_test"

 _common_attrs = {
     "requirements_in": attr.label(mandatory = True, allow_single_file = True),
@@ -25,7 +26,7 @@ def _uv_pip_compile(ctx, template, executable):
             "{{requirements_txt}}": ctx.file.requirements_txt.short_path,
             "{{resolved_python}}": py_toolchain.py3_runtime.interpreter.short_path,
             "{{python_platform}}": _python_platform(ctx.attr.python_platform),
-            "{{label}}": str(ctx.label),
+            "{{label}}": str(ctx.label).removesuffix(_DIFF_TEST_SUFFIX),
         },
     )

@@ -84,7 +85,7 @@ def pip_compile(name, requirements_in = None, requirements_txt = None, target_co
     )

     _pip_compile_test(
-        name = name + "_diff_test",
+        name = name + _DIFF_TEST_SUFFIX,
         requirements_in = requirements_in or "//:requirements.in",
         requirements_txt = requirements_txt or "//:requirements.txt",
         python_platform = python_platform or "",

The benefit is we keep the suffix manipulation in the same file, but being completely paranoid, this could accidentally remove _diff_test from a user provided label name.

@@ -25,6 +25,7 @@ def _uv_pip_compile(ctx, template, executable):
"{{requirements_txt}}": ctx.file.requirements_txt.short_path,
"{{resolved_python}}": py_toolchain.py3_runtime.interpreter.short_path,
"{{python_platform}}": _python_platform(ctx.attr.python_platform),
"{{label}}": str(ctx.label),
Copy link
Contributor

@mark-thm mark-thm May 21, 2024

Choose a reason for hiding this comment

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

thought's on passing an explicit value for this in the macro instead of doing the substitution of the _diff_test suffix in bash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that the alternative I shared above? Or do you refer to something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant something a little different -- we can take an explicit attr that is the label to use for re-generating the files and set it correctly in both the compile and test targets rather than performing replacements on the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I fully understand that. Do you want user to set that attr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean something like this:

diff --git a/uv/private/pip.bzl b/uv/private/pip.bzl
index 9699fd7..d00e21f 100644
--- a/uv/private/pip.bzl
+++ b/uv/private/pip.bzl
@@ -3,6 +3,7 @@
 _PY_TOOLCHAIN = "@bazel_tools//tools/python:toolchain_type"

 _common_attrs = {
+    "original_name": attr.string(mandatory = True),
     "requirements_in": attr.label(mandatory = True, allow_single_file = True),
     "requirements_txt": attr.label(mandatory = True, allow_single_file = True),
     "python_platform": attr.string(default = ""),
@@ -25,7 +26,7 @@ def _uv_pip_compile(ctx, template, executable):
             "{{requirements_txt}}": ctx.file.requirements_txt.short_path,
             "{{resolved_python}}": py_toolchain.py3_runtime.interpreter.short_path,
             "{{python_platform}}": _python_platform(ctx.attr.python_platform),
-            "{{label}}": str(ctx.label),
+            "{{label}}": str(ctx.label.relative(ctx.attr.original_name))  # this is a deprecated api in 7.1+
         },
     )

@@ -77,6 +78,7 @@ def pip_compile(name, requirements_in = None, requirements_txt = None, target_co

     _pip_compile(
         name = name,
+        original_name = name,
         requirements_in = requirements_in or "//:requirements.in",
         requirements_txt = requirements_txt or "//:requirements.txt",
         python_platform = python_platform or "",
@@ -85,6 +87,7 @@ def pip_compile(name, requirements_in = None, requirements_txt = None, target_co

     _pip_compile_test(
         name = name + "_diff_test",
+        original_name = name,
         requirements_in = requirements_in or "//:requirements.in",
         requirements_txt = requirements_txt or "//:requirements.txt",
         python_platform = python_platform or "",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it is better as:

            "{{label}}": "@@%s//%s:%s" % (
                ctx.label.workspace_name,
                ctx.label.package,
                ctx.attr.original_name,
            ),

There is a new API Label.same_package_label but not available in 7.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this approach is simpler but I could have misunderstood. Please feel free to update this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean something like the original_name example, but see the tradeoffs you highlight with flux around label functionality and am happy to take your contribution as is. Thanks for exploring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

@mark-thm mark-thm merged commit 6b5a7cb into theoremlp:main May 22, 2024
7 of 9 checks passed
@honnix honnix deleted the generate-header branch May 22, 2024 20:09
thm-automation bot pushed a commit that referenced this pull request May 22, 2024
Follow-up to #35: pass the label of the generator target to the test target explicitly. In #35 we went with a solution that messed with the label string, but passing an explicit label should be safer w/r/t refactors.
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