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.

Sign up for GitHub

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

Jump to bottom

Set PATH using real SDK and NDK directories #2583

Merged
merged 2 commits into from
Apr 27, 2022
Merged

Set PATH using real SDK and NDK directories #2583

merged 2 commits into from
Apr 27, 2022

Conversation

dbnicholson
Copy link
Contributor

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

@misl6
Copy link
Member

misl6 commented Apr 19, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

misl6 commented Apr 23, 2022 •
edited
Loading

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

@dbnicholson
Copy link
Contributor Author

Sure, I'll take a look on Monday.

@dbnicholson
Copy link
Contributor Author

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

@dbnicholson
Copy link
Contributor Author

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
Correct recipe test assertion
783ea19 
`Arch.find_executable` uses the context environment `PATH` rather than
the process environment `PATH`, so change the assertion to test the
correct value. At present these are the same in this test, but since
f7f8cea `PATH` is updated in the
`Context.env` attribute by `Context.prepare_build_environment` instead
of `os.environ`. If `Recipe.get_recipe_env` or `Arch.get_env` were
changed to call `prepare_build_environment`, this test would fail.
@dbnicholson
Copy link
Contributor Author

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
misl6 previously approved these changes Apr 26, 2022
View reviewed changes
Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

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 :)

tests/test_recipe.py Show resolved Hide resolved
tests/test_build.py Show resolved Hide resolved
@dbnicholson
Set PATH using real SDK and NDK directories
0ef906f 
This is a regression from f7f8cea.
Previously, `self.sdk_dir` and `self.ndk_dir` were passed to
`select_and_check_toolchain_version`.
@dbnicholson
Copy link
Contributor Author

I added the comment, ready to review again.

@misl6
Copy link
Member

misl6 commented Apr 27, 2022

I added the comment, ready to review again.

Thank you!

@misl6 misl6 merged commit 92cb8f4 into kivy:develop Apr 27, 2022
@dbnicholson dbnicholson deleted the real-sdk-ndk-dirs branch May 16, 2023 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@misl6 misl6 misl6 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dbnicholson @misl6