fix: ignore_root_user_error should be taken from root module only

[shabanzd]

Only the root module should be able to decide ignore_root_user_error. Modules being depended upon don't know the final environment, so they aren't in the right position to know or decide what the correct setting is. This PR implements a solution to take the ignore_root_user_error value from the root module only.

Fixes #1658

[aignas]

Thanks for the PR!

[rickeylev]

Also, remember the root might might not explicitly setup any python toolchains; it might only be getting one because of some transitive module that needs python.

And yes, only the root module should be able to set ignore_root_user_error. A module being used as a library doesn't know what the final runtime environment is, so it can't know what the correct decision is.

[aignas]

LGTM, but it needs a CHANGELOG snippet and it wouldbe great to have the PR description to be a nice summary of the change as well as it will be used as a commit message when the PR gets merged.

[aignas]

@shabanzd could you think of a way to add an integration test for this? The main branch has support for bazel-in-bazel integration tests now and you could add one in tests/integration that would exercise the modified code.

[rickeylev]

I originally requested tests when Ignas asked me about this PR, but I'm not so sure we should block on that now. The only idea I have for testing this is to add some print statements and check their output in the integration test. The prints would be guarded by an environment variable. However, doing that requires setting up a new test runner for the integration tests (one that can capture output and assert on it), which is maybe a bit too much to ask a new contributor in this PR.

Any alternative testing ideas that might be easier?

[ziad-fernride]

I would agree with @aignas that maybe the simplest way would be to have bazel-in-bazel integration tests with different scenarios and check for failures/successes based on the scenario ?

[rickeylev]

Ah, that gives me an idea: write an extra file somewhere with some info about what the extension did. Guard this behind an environment variable. Then we can write a test inside the integration test that reads that file and verifies its content. Maybe have the python repository repo rule write out what its settings were? Then it should be easy to use_repo() it and locate the file within it.

[rickeylev]

I added an integration test to verify behavior. It works as I described last: the extension checks an environment variable and writes a file with some debug info. A test reads it to verify what happened. Overall it works pretty well and isn't very invasive; we can probably use this same technique to test other subtle cases with the bzlmod extension code.

[shabanzd]

Sorry was offline since Thursday, thanks for taking care of the testing @rickeylev 🙏🏼🙏🏼