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.

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

cmake: Fix SYS_BL_UPDATE param (Sponsored by CubePilot) #23158

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

julianoes
Copy link
Contributor

It turns out that the rc.board_bootloader_upgrade file was removed each time.

By no longer doing the remove_directory call, we will probably accumulate files that are removed over time without a clean build but I'd think we can live with that.

Fixes #20592

@@ -121,7 +121,6 @@ add_custom_command(
${romfs_gen_root_dir}/init.d/rc.autostart
${romfs_gen_root_dir}/init.d/rc.autostart.post
${romfs_copy_stamp}
COMMAND ${CMAKE_COMMAND} -E remove_directory ${romfs_gen_root_dir}/*
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, did you verify this is still working cleanly with ROMFS changes and incremental rebuilds?

Copy link
Member

Choose a reason for hiding this comment

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

I believe the proper solution to all of this would be to have 1-1 dependencies for every file, then everything would actually work as intended, including clean, and other subtle dependencies.

The blocker is that airframes can be tagged excluded (processed through python scripts) and the actual build system is oblivious to this.

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 assume it won't work cleanly with ROMFS changes. That's what I wrote in my message.

Copy link
Contributor Author

@julianoes julianoes May 21, 2024

Choose a reason for hiding this comment

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

I honestly don't know exactly the way you want to fix this all properly. It's all a bit of a mystery to me.

So, if you come with an alternative fix in the next few days, fine, otherwise I suggest we merge this, or remove the param and the functionality. It's currently broken and we need to either fix or remove it.

Copy link
Member

Choose a reason for hiding this comment

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

Let's try to avoid leaving something in a broken state in exchange for a quick fix. I won't be able to take a closer look until Thursday, but if you need a short term fix you could try including the init logic by default, just be careful it fails gracefully if the param or file doesn't exist.

Copy link
Member

Choose a reason for hiding this comment

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

Would we be able to make the ability to add files in a board init folder automatic? If it exits copy it in to the romfs. Then a script in the boards init files could reference the files and allow for a more structured init, and shared init files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll leave this to you to fix then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dagar reminder that you wanted to fix this properly...

@julianoes julianoes closed this May 21, 2024
@julianoes julianoes deleted the pr-fix-sys-bl-update branch May 21, 2024 22:24
@julianoes
Copy link
Contributor Author

Should we reconsider this fix?

@julianoes julianoes restored the pr-fix-sys-bl-update branch October 10, 2024 02:42
@julianoes julianoes deleted the pr-fix-sys-bl-update branch October 10, 2024 02:42
@julianoes julianoes restored the pr-fix-sys-bl-update branch October 10, 2024 02:45
@julianoes julianoes reopened this Oct 10, 2024
@julianoes
Copy link
Contributor Author

julianoes commented Oct 10, 2024

@dagar I'm reopening this because you have not suggested an alternative yet as you said you would. Sorry to be snarky like that but I find it frustrating to have a fix blocked for something that's just completely broken.

It turns out that the rc.board_bootloader_upgrade file was removed each
time.

By no longer doing the remove_directory call, we will probably accumulate
files that are removed over time without a clean build but I'd think we
can live with that.
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.

rc.board_bootloader_upgrade isn't compiled in. No SYS_BL_UPDATE
3 participants