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

Avoid default value for TCVERSION (followup to #5390) #5417

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

hgy59
Copy link
Contributor

@hgy59 hgy59 commented Sep 6, 2022

Description

The error was added by #5390

  • avoid to set a default value for TCVERSION, as it might be evaluated later
  • fixes the build of packages with REQUIRED_MIN_DSM (like mkvtoolnix, nodejs_v16)

Fixes mkvtoolnix and nodejs_v16 builds in #5414

Type of change

  • Bug fix
  • New Package
  • Package update
  • Includes small framework changes
  • This change requires a documentation update (e.g. Wiki)

- we must not set a default value for TCVERSION, as it might be evaluated later
- fixes the build of packages with REQUIRED_MIN_DSM (like mkvtoolnix, nodejs_v16)
@hgy59 hgy59 added the framework label Sep 6, 2022
@hgy59
Copy link
Contributor Author

hgy59 commented Sep 6, 2022

@th0ma7 it is always a mystery how make variables get evaluated...

@th0ma7
Copy link
Contributor

th0ma7 commented Sep 7, 2022

@hgy59 I played quite a bit with that and still there's a lot of black magic going on. But I'll try to explain my findings (and please feel free to correct me as it goes).

First off, depending on how the make call is being made, it will result in multiple (re)evaluation of the variables. For instance, calling make all-supported ends-up calling in parallel make arch-<ABC>-X.Y while emptying it's environment variables (although it does pass arguments). This in turns calls make TCVERSION=<version> ARCH=<arch>, again without passing any environment variables (e.g. -@MAKEFLAGS). But, forgot to mention, this in turns will also be evaluated through dependency-list and dependency-flat as they are used to make all native dependencies first and only then move on with the parallel building.

So lets take the version_gcc call that I recently removed (#5401) as this was too error prone, and lets say it is in a given Makefile: before getting anything done, 5x evaluation of that resulting value will have occured when using make all-supported, all resulting in bogus return value as the toolchain hasn't yet been installed nor a tc_var.mk generated (and tc_vars.cmake for what's worth). Now here's the trick with version_gcc in particular, if you are re-invoking over an existing build (e.g. with an existing $(WORK) directory) the environment is still there and therefore the return value may well be right this time :)

Now is the interesting part, the environment such as ran using $(RUN) will have all you need in there e.g. you can use and abuse of any variable using a shell ${TC_GCC}. Now the question is: when can I make use of this? The only "safe" place is after the include ../../mk/spksrc.spk.mk or include ../../mk/spksrc.cross-*.mk. Reason is: it will have processed the spksrc.tc.mk, resulting in a valid environment at that stage only. The caveat is that this might not meet all the usecases you have in mind where you wanted to validate something really early in your makefile.

So in turns this then mean that you would only adjust dependencies based on a shell environment value after the include. Doing it before then may result in odd results.

Now, for the exception part, TCVERSION and ARCH. Theses gets evaluated really early in the process as they are being used to locate and make use of the toolchain/syno-<arch>-<version>. Also, theses gets evaluated as Makefile variables using $(ARCH) and $(TCVERSION) to the contrary of tc_vars.mk variables which are shell based.

In your usecase for your patch, the value of $(TCVERSION) will be empty by default when called using make all-supported or make arch-<arch>-<version> until it calls itself once more transforming it as make TCVERSION=<version> ARCH=<arch>. Question is: will setting a default value be usefull or change it's behavior? probably not. Reason is, it isn't needed for all-supported as it will gather the proper arch list values from SUPPORTED_ARCHS and AVAILABLE_TCVERSIONS from spksrc.common.mk and recall itself using multiple time in parallel using arch-<arch>-<version>. And is it needed for the later? no again as it will extract that info from the arguments passed to recall itself with make TCVERSION=<version> ARCH=<arch>. So being empty is OK until it's not.

Hope this helps or have I muddied the water?

@hgy59
Copy link
Contributor Author

hgy59 commented Sep 7, 2022

@th0ma7 thanks a lot for the detailed findings (and writing it down).

BTW: my intent was to initialize TCVERSION only for noarch builds. But it affected the prechecks for REQUIRED_MIN_DSM.

@hgy59 hgy59 merged commit 5542161 into SynoCommunity:master Sep 7, 2022
@hgy59 hgy59 deleted the fix_spk_mk branch September 7, 2022 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants