-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
framework: refactor common.mk to split base definitions #6906
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
Conversation
Per hgy59 suggestion, split plain macros with no directory dependencies into a new spksrc.base.mk file: - SHELL, empty, space, MSG, LANGUAGES - Terminal colors (RED, GREEN, NC) - Version comparison macros (version_le, version_ge, version_lt, version_gt) - uniq, dedup, merge utility macros Both directories.mk and common.mk now include base.mk, ensuring macros are available regardless of include order (fixes cases where common.mk is included before directories.mk).
|
I wonder if we shouldn't make a larger change and migrate to There may be a caveat though with: And migrate to |
|
for that would be worth adding |
|
hey @th0ma7, I was hoping to be able to merge this and am unclear on the scope expansion you have proposed. I did build locally the Thoughts on next steps? |
|
We'd need to go back at compiling nasm like we did previously as a native dependency... At least temporarily until we remove it all together as already available in the container now since our last debian upgrade. @hgy59 already did some work on that but needs to be completed. I might take a stab at it over the coming weeks/months but in the meantime rebuilding a native nasm is the shortcut... Unless we can fake it and just have a symlink from native nasm install to the container nasm...? Worth a try i guess... |
|
@mreid-tt I took time to revisit this PR, while in theory simple it does have a few implications. I am unsure of the way to go, but with recent changes included in #6914 I think we should reconsider a few things...
Caveat:
|
Note that half of the makefiles where already using common.mk. Now standardizing so all uses it.
|
@mreid-tt I had time today to take a stab at it - tested against EDIT: Noting that there are toooo many cross makefiles updated to use |
|
While not related I hapend to have noticed that the pre-built native/llvm-14.0 had a runpath error on |
|
@mreid-tt I confirm that the following do build ok:
From my perspective this is good to go. |
|
@mreid-tt the description of the PR would need updating though... if you have any spare cycles to do so. |
|
@mreid-tt I had a few more spare cycles and was able to update the PR description. Let me know if anything else needs adjusting. |
Description
Refactors
spksrc.common.mkinto a structured set of focused include files to improve maintainability, clarity, and include-order safety. Refactors theWORK_DIR/version_gedependency solution from #6901 by enhancing @hgy59's suggested approach by further dividing the mk file into a sub-directory tree.Problem:
spksrc.directories.mkusesversion_ge(line 67) butversion_gewas defined inspksrc.common.mk. Whendirectories.mkwas included beforecommon.mk, the macro wasn't available, causing silent failures in version conditionals.Solution:
This change decomposes
spksrc.common.mkinto clearly scoped components under mk/spksrc.common/, while preserving existing behavior:spksrc.common.mk now acts as the single aggregation entry point and is responsible for loading these components in a consistent and well-defined order.
Version comparison macros (version_le, version_ge, version_lt, version_gt) are now clearly defined in macros.mk
uniq,dedup,merge) are isolated inmacros.mkarchs.mklogs.mkIMPORTANT: This PR addresses a long-standing issue where previously
spksrc.archs.mkwas being used pre-spksrc.cross-cc|cmake|meson|etc.mkand was being slowly migrated over to directly callingspksrc.common.mk. This PR migrate all cross & spk to usingspksrc.common.mkonly.Testing:
cross/fossil-scm(x64-7.1) ✓spk/minio(x64-7.1) ✓spk/borgbackup(x64-7.1) ✓toolchain/syno-x64-7.1✓spk/python312(x64-7.1, aarch64-7.1)spk/python312-wheels(aarch64-7.1)spk/synocli-videodriver(x64-7.1)spk/ffmpeg(x64-7.1)spk/tvheadend(x64-7.1)Closes #6901
Checklist
all-supportedcompleted successfullyType of change