-
Notifications
You must be signed in to change notification settings - Fork 5
Simplified Build Instructions for Linux #3
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.
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
base: main
Are you sure you want to change the base?
Conversation
5dea390
to
055b983
Compare
…sted in a fresh Ubuntu VM.
055b983
to
3d204bb
Compare
I like what you've done here, these are great details and some nice additions! I am a little wary about having platform specific build scripts in the template though. Since it's a cross-platform project, I generally prefer to have all the tools present be cross-platform as well? How about this, could you move the install .sh script to the /android folder so it's at least out of the root, and rename it to reflect its limitations? Like a "linux_apt_install_android_deps.sh" or "ubuntu_install_android_deps.sh" or that sort of thing. I'm a little less sold on the build_android.sh script, among other things it does both configure and build every time. Configure should only happen once, or it will add to the build time. This configure step can also be achieved via cmake presets, which would be preferable to me since it uses a more consistently cmake interface that integrates better with existing tools such as VSCode's Cmake plugin. I considered going this route for a bit, but I've been uncertain if Cmake presets are actually a tool designed for the code provider or the code consumer, since they can get a bit specific. Running the Android targets was intended to be simple enough to run via command line, but also integrates nicely with VSCode's Cmake plugin. I'm not sure this part benefits well from a generic wrapper, since it obfuscates a pretty simple command! I think this is one that devs can build workflow specific shell scripts if they need them. |
Okay, I moved the dependency installer! It also prompts now to setup the envs permanently! My main idea with the build script was that it only temporarily setups the envs, because it's possible that this will overwrite the users current configuration... |
@@ -1,3 +1,6 @@ | |||
# Ignore all shell scripts in the root directory | |||
/*.sh | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably shouldn't be in the template by default. You've actually still got install_android_deps.sh in the root of this commit, likely because of this line!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to effectively prevent new sh files to be added to the main directory, but not the current one - that has been deleted now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looks like you got install_android_deps.sh now.
Keeping .sh files out of the root directory is only to keep the template tidy for developers! IMO, templates should always have pretty clear and universally relevant roots. Devs can put whatever files they want in the root once they start developing, so I don't want to block inclusion of .sh files here.
dee65b8
to
a1006af
Compare
…pt and update .gitignore to include shell files in the root dir.
a1006af
to
936940e
Compare
This will simplify the setup for linux alot I think. It compiles on Ubuntu now all the time. Tested it in a VM.