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

load board config again to allow overwrite family config #4643

Closed
wants to merge 2,881 commits into from

Conversation

hzyitc
Copy link
Member

@hzyitc hzyitc commented Jan 2, 2023

Description

The board config is only contain some variable assignments, so it should be safe to be sourced twice. If it doesn't, we can fix easily.

With this, we can overwrite the family config. So we don't need to check BOARD in family config ( e.g. the u-boot config ) . Just define it in board config.

How Has This Been Tested?

[ ] Build and run test for all board

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@igorpecovnik
Copy link
Member

@rpardini This is another NEXT topics :) We used to have this override and once we let it go. In NEXT?

@mhoffrog
Copy link
Member

mhoffrog commented Jan 2, 2023

This is another NEXT topics :) We used to have this override and once we let it go. In NEXT?

Including a script twice in different stages of the build is raising another level of confusion and hidden dependencies.
So just as a draft thinking I would prefer this re-design to be something like:

  1. include the board script only once within config-prepare.sh - as is today
  2. Let the board script provide two functions - like e.g.:
    a) board_pre_main_config() - this one will be called from config-prepare.sh right after including board script
    b) board_post_main_config() - this will be called appropriately after any familiy and/or common scripts have been included from the place you did locate the second source including of this PR.

For backward compatibility we should check the existence of those functions and log a warning, if those are not there to motivate for according re-factoring of the board scripts over time.

@hzyitc
Copy link
Member Author

hzyitc commented Jan 3, 2023

I don't think spilting into two parts is a good idea.

  1. If a variable is defined in pre, when someone try to add this variable in family config, he can't known that some board config will be overwrite.
  2. If we need to overwrite a function, we need to define this function in a function, which is really strange.

The first include only require one varable - BOARDFAMILY.

So if we refactor, we can group the boards in folder which named as its family. So we can solve this problem and make struct more clean.

@mhoffrog
Copy link
Member

mhoffrog commented Jan 3, 2023

So if we refactor, we can group the boards in folder which named as its family. So we can solve this problem and make struct more clean.

Sounds good to me as well - the simpler the better!

@rpardini
Copy link
Member

rpardini commented Jan 4, 2023

Folks, we've the extensions mechanism to address this and others.
Example:

# Override family config for this board; let's avoid conditionals in family config.
function post_family_config__orangepi5_use_vendor_uboot() {
BOOTSOURCE='https://github.com/orangepi-xunlong/u-boot-orangepi.git'
BOOTBRANCH='branch:v2017.09-rk3588'
BOOTPATCHDIR="legacy"
}

Docs (a bit outdated): https://docs.armbian.com/Developer-Guide_Extensions/

armbian-next has a few more hooks (mostly cosmetical stuff, to avoid using case / if for BRANCH etc) but it's mostly the same.

@mhoffrog
Copy link
Member

mhoffrog commented Jan 4, 2023

Copy link
Member

@igorpecovnik igorpecovnik left a comment

Choose a reason for hiding this comment

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

Lets rather move this to extensions.

@rpardini rpardini mentioned this pull request Jan 31, 2023
6 tasks
@igorpecovnik igorpecovnik added the Work in progress Unfinished / work in progress label Feb 21, 2023
@igorpecovnik
Copy link
Member

Still relevant?

@igorpecovnik igorpecovnik force-pushed the allow-board-config-overwrite-family-config branch from 2f381c5 to 15de9cf Compare May 19, 2023 16:41
@igorpecovnik igorpecovnik added Backlog Stalled work that needs to be completed and removed 23.08 Work in progress Unfinished / work in progress labels Nov 15, 2023
@ColorfulRhino
Copy link
Collaborator

Follow-up PR: #6801

@ColorfulRhino ColorfulRhino deleted the allow-board-config-overwrite-family-config branch June 30, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backlog Stalled work that needs to be completed
Development

Successfully merging this pull request may close these issues.