doc: Minor details to documentation for building A-S#7477
Conversation
| ```shell | ||
| export ANDROID_HOME=~/.mozbuild/android-sdk-linux | ||
| export ANDROID_SDK_ROOT=~/.mozbuild/android-sdk-linux | ||
| ``` |
There was a problem hiding this comment.
I'm not actually totally sure if the ANDROID_SDK_ROOT set is necessary here, but a later step here describes it being set here and it's not mentioned elsewhere on the page.
In the codebase I don't see any other uses of ANDROID_SDK_ROOT (aside from documentation or CI setting it) so I assume its deprecated/unused. If so, I can instead adjust the other step to remove the reference to this.
There was a problem hiding this comment.
I never set ANDROID_SDK_ROOT just ANDROID_HOME. How about removing the reference from that other doc rather than adding one here?
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
This is great, thanks for keeping these fresh. I had a couple suggestions, but feel free to push back on any/all of them.
| 1. Recent brew installations of Python do not contain a `python`/`pip` symlink. You may be able to run the following to create them: | ||
| ```shell | ||
| brew uninstall python3 | ||
| brew install python3 |
There was a problem hiding this comment.
If the recent installations don't contain pip then how does uninstalling and re-installing fix it? I don't doubt that this works, I'm just curious why since I'm not a brew user.
There was a problem hiding this comment.
I'm actually a little bit surprised it worked as well. I had assumed it was because of brew install python vs brew install python3, but on checking now, these are aliases. The reinstallation step in particular is noted here. it worked perfectly for me, but on reflection here it's unclear why- I have a couple thoughts but I'm unsure so I'll make a note and examine them later.
The source issue about the symlinks is pretty clear, though, and brew info python shows the intended symlinks, so for now I've changed my comment to give a better + more idiomatic solution more in line with what brew intends.
| 1. More recent version of Python (3.12+) have deprecated `distutils`, a dependency of the above script. If you encounter a ModuleNotFoundError, you can run first: | ||
| ```shell | ||
| brew install python-setuptools | ||
| ``` |
There was a problem hiding this comment.
I'm pretty sure I've seen this same issue, how about making this a bit more generic? I'm thinking something like "If you encounter a ModuleNotFoundError, then install setuptools on your system (for example brew install python-setuptools)"
| ```shell | ||
| export ANDROID_HOME=~/.mozbuild/android-sdk-linux | ||
| export ANDROID_SDK_ROOT=~/.mozbuild/android-sdk-linux | ||
| ``` |
There was a problem hiding this comment.
I never set ANDROID_SDK_ROOT just ANDROID_HOME. How about removing the reference from that other doc rather than adding one here?
| ```bash | ||
| rm ../firefox-ios/MozillaRustComponents/Sources/MozillaRustComponentsWrapper/Generated/glean_sym.swift | ||
| ``` | ||
|
|
There was a problem hiding this comment.
I've added this step per recent discussion. We could also the cp and rm in one command with something like:
find /some/folder -maxdepth 1 -name '*.swift' ! -name 'glean_sym.swift' -exec cp {} /other/folder/ \; but I like the clarity of separating them. Let me know what you think, I'm not tied to it.
There was a problem hiding this comment.
Agreed, I like this way of presenting it better as well.
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
Looks great, I think we're ready to merge this.
| ```bash | ||
| rm ../firefox-ios/MozillaRustComponents/Sources/MozillaRustComponentsWrapper/Generated/glean_sym.swift | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Agreed, I like this way of presenting it better as well.
Co-authored-by: bendk <bdk@mozilla.com>
2 participants
This PR makes no changes aside from minor documentation additions for setup, so doesn't need changelog updates or any breaking changes (or changes at all for that matter).
Pull Request checklist
[ci full]to the PR title.