fix(windows): symlink bootstrap script when not building zip

[KoltesDigital]

This fixes #1840 by supporting again --build_python_zip=false on Windows.

When the zip file isn't build, the transition executable looks for the eponymous bootstrap script but the latter doesn't exist. I've just added a symlink, and refactored a bit because logic would have been duplicated.

It seems you don't run CICD on Windows. FWIW I've manually tested it, both with build_python_zip as true and false.

[rickeylev]

Ah, this is in transition.bzl? So it's the combination of windows + build_zip=false + multi-version rules. Yeah, I think I understand what's going on here. For windows builds, it creates a foo.exe, which then looks for foo adjacent to it, then runs python foo

The to_list() call is problematic. That's going to flatten the runfiles, which is expensive. An alternative might be to look at the default outputs (DefaultInfo.files, not runfiles) instead. IIRC, the "second stage" file (foo or foo.zip) is also a default output.

Or, actually, we should be able to check the flag and use that to decide.

Do you need this to work with Bazel 6, or Bazel 7 with the pystar rules disabled? If not, then we have some more options to fix this.

no windows CI

We have a windows CI, but yeah, our windows support is a bit spotty, and our integration tests are a bit light. An analysis test for this should be doable, though

[KoltesDigital]

@rickeylev thanks for the review!

I'm still new to the multi version thing, I think the transition rule applies the version selection and need an output, so it has been chosen to just symlink the output of the actual rule. Hence the error, because the bootstrap script wasn't symlinked.

And I'm new to the whole repo as well. I can try changing the logic as you suggested, but it may be out of my understanding.

Regarding the call to to_list(), well it was already there, I've just moved it a few lines up to avoid a second call!

I'm on Bazel 7+ and I have no idea what pystar is.

My .bazelrc merely contains startup --windows_enable_symlinks and build --enable_runfiles.

Agreed for the CI, I should have read something unrelated. I guess analysis test means a test that triggers a rule. Where are they located?

[KoltesDigital]

@rickeylev updated as you suggested:

• files are indeed listed in default outputs,
• flag value is retrieved to decide which branch to go through.

[rickeylev]

I pushed a basic analysis test, this allows checking the outputs being produced. It currently fails because, well, it doesn't look like the outputs are correct. I think this is behavior existing before your PR, though, and I'm surprised this works at all, zip true or false, for windows?

In any case, we don't need to fully address that in this PR (out of scope).

The way the logic is written is a bit hard to follow, though, so I'd like that cleaned up a bit; see inline comment

[KoltesDigital]

@rickeylev well thanks for having done it :)

Your last commit left both logic in place, so I've continued the refactor a bit further. Agreed it's more readable. Feel free to squash.

Going further, I was wondering why the bootstrap script should not be executable, because it starts with a shebang. Even if it's ignored on Windows. I realized that both targets are generated as executable, so I removed the variable.

[KoltesDigital]

I'm worried by the CICD: Error: 'py' value has no field or method 'build_python_zip', where could it come from?

[rickeylev]

mark both as executable because both are generated as executable

Good observation! SGTM

build_build_zip attribute error

Oh yeah. That is because the py.build_python_zip fragment attribute isn't available in Bazel 6, only bazel 7+. Normally this is easily worked around by using select+config_setting, but I'm not sure it'll be as easy in this case. This issue is the flag's default value, "auto", is determined by the host Bazel is running on src. Bazel 6 doesn't have an API to detect the host Bazel OS, so we don't have a way to deduce what the effective flag value is. So, that's annoying.

I think we could infer the state based on if there is a .zip in the inner target's default outputs? Or just fix this for Bazel 7.

[KoltesDigital]

Thanks for the explanation!

Regarding what to do now, well, you're contributor and I'm only user so long term view is yours. But it seems I've been the only one to stumble across the bug so far, and I'm on 7+, so I'm fine with the later choice. If someone later complains on 6, just say "meh, upgrade". Kind of kidding, but as far as I remember, I haven't noticed the upgrade to 7, it just worked as before. Whereas the migration to bzlmod required some work, but 7 still supports the WORKSPACE way.

[criemen]

As a user that's blocked from upgrading to a later version of rules_python (and, therefore, checking in our bzlmod lockfile, as rules_python in our older version still produces platform-specific entries), I'd be way more interested in getting this fix in and have it only work on bazel 7.

For us, --build_python_zip=false is unfortunately a non-negotiable feature that's blocking this rules_python upgrade.

[KoltesDigital]

@criemen until the PR is accepted and a new version is released, I'm already using it on prod using a local registry. You could also use a local override with a git submodule. At least two temporary ways to allow you to upgrade.

[aignas]

You can also use single_version_override and apply this PR as a patch to unblock your move to bazel 7, etc.

…

On 5 July 2024 08:16:00 GMT+09:00, "Jonathan Giroux (Koltes)" ***@***.***> wrote: @criemen until the PR is accepted and a new version is released, I'm already using it on prod using a local registry. You could also use a local override with a git submodule. At least two temporary ways to allow you to upgrade. -- Reply to this email directly or view it on GitHub: #2015 (comment) You are receiving this because your review was requested. Message ID: ***@***.***>

[rickeylev]

I made the implementation work with both Bazel 6 and Bazel 7. I had to lighten the tests for Bazel 6, but that's NBD. Let's see what CI thinks.

[KoltesDigital]

@aignas talking about single_version_override, I can't use it because for other reasons I need to patch MODULE.bazel, but the patched content is not taken into account. In particular, it adds a new dependency using use_repo, but the patched rules can't access this dependency, so I guess the dep graph is not calculated again after the patch... However it is using local_path_override! Is it by design that the behavior differs between these two overrides?

BTW, I noticed the central registry is late on the versions. 0.34.0 has been released a few days ago, but https://registry.bazel.build/modules/rules_python is still at 0.33.2.