Set PATH using real SDK and NDK directories

[dbnicholson]

This is a regression from f7f8cea.
Previously, self.sdk_dir and self.ndk_dir were passed to
select_and_check_toolchain_version.

[misl6]

Ouch, I missed that! Agree about these changes.
What about line 374 ?
Maybe we should add a test or a comment that cover this case?

[dbnicholson]

Ouch, I missed that! Agree about these changes. What about line 374 ? Maybe we should add a test or a comment that cover this case?

Whoops, missed that line as I was focused on the f-strings. Let me see if I can make a test for this.

[dbnicholson]

Updated now with the missing line and a minimal test. It passed locally, but this is my first time in this test suite so I might not be doing the right thing.

[misl6]

Unfortunately the tests are failing, can you take care of it? (The pytest ones, others on macOS are expected to fail)

[dbnicholson]

Sure, I'll take a look on Monday.

[dbnicholson]

Hmm, it passes when I run only tests/test_recipe.py. Some other test must be altering PATH without cleaning up.

[dbnicholson]

Oh, I see what's going on now. This actually comes from f7f8cea, but my added test exercises it. Prior to that commit, select_and_check_toolchain_version would alter os.environ. After that commit, the Context class attribute env is altered.

Since the test I added runs Context.prepare_build_environment, the global Context.env class attribute is altered for any subsequent test. I can think of 3 possible fixes:

• Change Context.prepare_build_environment to alter os.environ like it used to.
• Change my new test to mock access to Context.env so prepare_build_environment doesn't affect subsequent tests
• Change env to be a Context instance attribute instead of a class attribute. I'd argue this is the most proper, but I don't know if there are other implications of it.
• Change test_get_recipe_env_with so it compares against self.ctx.env['PATH'] instead of os.environ['PATH']. I think this change should be made regardless since that's actually what Arch.get_env does. Whether self.ctx.env and os.environ are the same is an orthogonal check.

[dbnicholson]

I think this should be fixed now. I changed the new test so it patches Context.env and also corrected test_get_recipe_env_with so it compares against ctx.env instead of os.environ even though it's not needed with the patching.

[misl6]

LGTM, thank you!

I've just added a minor suggestion.
I will wait to merge, so you can address it, but not consider it as a blocking :)

[dbnicholson]

I added the comment, ready to review again.

[misl6]

I added the comment, ready to review again.

Thank you!