Skip to content

Install pre-compiled files into source tree #242

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

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

will-v-pi
Copy link
Contributor

This adds an install step in the external projects when USE_PRECOMPILED=false to copy the compiled binaries into the source tree.

This also adds a new action to check that the pre-compiled files have been updated correctly, although I'm not sure if we should add that or not, as the files will change slightly depending on the compiler/SDK used so that action will probably fail quite a lot?

Fixes #240 (comment)

@lurch
Copy link
Contributor

lurch commented Jun 17, 2025

Looks like there are merge conflicts (which are probably what's also causing the CI failure?)

@will-v-pi
Copy link
Contributor Author

That CI failure is why I'm thinking this change doesn't want to have the check action, as the only change has been in the SDK so we don't really need to update the .elf

The conflict is just because the code for those files has been updated

Simplify CMakeLists.txt and BUILD.bazel skips

Install all ELFs as files, so they aren't marked as executable
@lurch
Copy link
Contributor

lurch commented Jun 18, 2025

Hmmm, something that's just occurred to me: if a change was made to a comment in one of the .c files, I guess that might result in no change to the generated ELF file; and the CI checks for that comment-fix PR might fail? 🤔 (but I guess in such a scenario we could just ignore the failing CI and merge anyway?)

@will-v-pi
Copy link
Contributor Author

Hmmm, something that's just occurred to me: if a change was made to a comment in one of the .c files, I guess that might result in no change to the generated ELF file; and the CI checks for that comment-fix PR might fail? 🤔 (but I guess in such a scenario we could just ignore the failing CI and merge anyway?)

Yeah, that's something to bear in mind, but I think that should be fine

CMakeLists.txt Outdated
if (USE_PRECOMPILED)
set(EXTERNAL_PROJECT_INSTALL_COMMAND "")
else()
set(EXTERNAL_PROJECT_INSTALL_COMMAND ${CMAKE_COMMAND} --install .)
Copy link
Contributor

Choose a reason for hiding this comment

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

"," gives me the willies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed that and added a better fix, with a new CMake function to handle the install vs don't install cases

This fixes the BUILD_ALWAYS issues as it's now only set when `USE_PRECOMPILED=false`, and also removes the need for `${CMAKE_COMMAND} --install .`
It's needed so if you update the binaries (eg `git pull`) they get re-copied and re-installed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants