edk2 icon indicating copy to clipboard operation
edk2 copied to clipboard

[Bug]: EmbeddedPkg/VirtualRealTimeClockLib fails to build due to SOURCE_DATE_EPOCH changes

Open mikkorapeli-linaro opened this issue 8 months ago • 6 comments

Is there an existing issue for this?

  • [x] I have searched existing issues

Bug Type

  • [ ] Firmware
  • [x] Tool
  • [ ] Unit Test

What packages are impacted?

EmbeddedPkg

Which targets are impacted by this bug?

No response

Current Behavior

Commit 6852f6984bdab86a1662e89e1ef0f3abc39559b6 added SOURCE_DATE_EPOCH handling to EmbeddedPkg/VirtualRealTimeClockLib. Sadly this does not work in yocto/open embedded build environments which do not export build tool "printenv" to the cross compile environments. Thus build of any configuration of edk2 with VirtualRealTimeClockLib fails with:

Building ... /home/mcfrisk/src/base/repo/build/tmp_zynqmp-kria-starter-psa/work/zynqmp_kria_starter_psa-poky-linux/edk2-firmware/202411/edk2/MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf [AARCH64]
/home/mcfrisk/src/base/repo/build/tmp_zynqmp-kria-starter-psa/work/zynqmp_kria_starter_psa-poky-linux/edk2-firmware/202411/edk2/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c: In function 'LibGetTime':
<command-line>: error: stray '`' in program
/home/mcfrisk/src/base/repo/build/tmp_zynqmp-kria-starter-psa/work/zynqmp_kria_starter_psa-poky-linux/edk2-firmware/202411/edk2/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c:89:20: note: in expansion of macro 'BUILD_EPOCH'
   89 |     EpochSeconds = BUILD_EPOCH;
      |                    ^~~~~~~~~~~
<command-line>: error: 'printenv' undeclared (first use in this function)
/home/mcfrisk/src/base/repo/build/tmp_zynqmp-kria-starter-psa/work/zynqmp_kria_starter_psa-poky-linux/edk2-firmware/202411/edk2/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c:89:20: note: in expansion of macro 'BUILD_EPOCH'
   89 |     EpochSeconds = BUILD_EPOCH;
      |                    ^~~~~~~~~~~
<command-line>: note: each undeclared identifier is reported only once for each function it appears in
/home/mcfrisk/src/base/repo/build/tmp_zynqmp-kria-starter-psa/work/zynqmp_kria_starter_psa-poky-linux/edk2-firmware/202411/edk2/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c:89:20: note: in expansion of macro 'BUILD_EPOCH'
   89 |     EpochSeconds = BUILD_EPOCH;
      |                    ^~~~~~~~~~~

The fallback to missing BUILD_EPOCH doesn't actually work and the date +%s command does not run:

 # Current usage of this library expects GCC in a UNIX-like shell environment with the date command
[BuildOptions]
  GCC:*_*_*_CC_FLAGS = -DBUILD_EPOCH=`printenv SOURCE_DATE_EPOCH || date +%s`

I've tried to fix this but have failed. The yocto/Open Embedded build environment always export SOURCE_DATE_EPOCH for reproducible builds but it seems to be impossible have a fallback for this missing variable in edk2 build configuration language. I did not find ways to export a shell script line which would detect a missing variable, without printenv that is. Would be nice if you developers could find a way to fix this. As a workaround I have proposed to always use the SOURCE_DATE_EPOCH variable in yocto environment but that is a patch needs to be maintained:

https://lists.yoctoproject.org/g/meta-arm/message/6424

Currently this fails builds of meta-arm based edk2-firmware 202411 and newer images for HW like AMD KV260.

Expected Behavior

Build passes.

Steps To Reproduce

checkout https://gitlab.com/Linaro/trustedsubstrate/meta-ts update meta-arm reference to include edk2-firmware update from 202408 to 202411 kas build kas/zynqmp-kria-starter-psa.yml

Build Environment

- OS(s): Ubuntu 24.04
- Tool Chain(s): yocto/OpenEmbedded

Version Information

202411

Urgency

Low

Are you going to fix this?

Someone else needs to fix it

Do you need maintainer feedback?

Maintainer feedback requested

Anything else?

No response

mikkorapeli-linaro avatar Apr 01 '25 06:04 mikkorapeli-linaro

@joeyli, please check this problem. It is caused by your change Commit https://github.com/tianocore/edk2/commit/6852f6984bdab86a1662e89e1ef0f3abc39559b6

@ardbiesheuvel , @pbatard , could you help confirm the change https://github.com/tianocore/edk2/commit/6852f6984bdab86a1662e89e1ef0f3abc39559b6

lgao4 avatar Apr 10 '25 00:04 lgao4

We should revert that change if it causes regresssions. Then, perhaps @mikkorapeli-linaro can help us find a solution that works with OpenEmbedded/yocto as well? I would prefer a solution where the BuildOption is evaluated conditionally, and can be superseded by a -D argument on the build command line.

ardbiesheuvel avatar Apr 10 '25 06:04 ardbiesheuvel

jonmason's patch works to me if I run 'export SOURCE_DATE_EPOCH=date +%s' before buliding RISC-V image.

joeyli avatar Apr 10 '25 07:04 joeyli

We should revert that change if it causes regresssions. Then, perhaps @mikkorapeli-linaro can help us find a solution that works with OpenEmbedded/yocto as well? I would prefer a solution where the BuildOption is evaluated conditionally, and can be superseded by a -D argument on the build command line.

The original purpose of $SOURCE_DATE_EPOCH is for image reproducible. Revert the change means that the reproducible is back to broken.

I prefer the above jonmason's patch, using ${SOURCE_DATE_EPOCH} instead of printenv, and remove date command:

edk2/EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf

  • GCC:__*_CC_FLAGS = -DBUILD_EPOCH=printenv SOURCE_DATE_EPOCH || date +%s
  • GCC:__*_CC_FLAGS = -DBUILD_EPOCH=${SOURCE_DATE_EPOCH}

The date command is for backward compatibility. But we can expose SOURCE_DATE_EPOCH environment manually before building image:

export SOURCE_DATE_EPOCH=date +%s

joeyli avatar Apr 10 '25 07:04 joeyli

@mdkinney is working out the solution for windows and linux build.

lgao4 avatar May 29 '25 01:05 lgao4

I have been looking at this issue for CLANG builds in Windows. Can not use linux shell specific commands when building under Windows. I like the idea of introducing a new environment variable that is available in both Windows and Linux environments and can be used in DSC/INF files as needed with $(EnvVarName) syntax. I recommend we add this new environment variable to the EDK II Build Specification and update the build command to set this environment variable each time a build is started so it contains the EPOCH when the build was started.

Following EDK II Build Specification naming conventions I propose:

EDK_SOURCE_DATE_EPOCH

This can be set in build python script using time module:

    # Set EDK_SOURCE_DATE_EPOCH to the current time in seconds at start of build
    os.environ["EDK_SOURCE_DATE_EPOCH"] = str(int(time.time()))

And can be used in VirtualRealTimeClockLib.inf with:

[BuildOptions]
  *_*_*_CC_FLAGS = -DBUILD_EPOCH=$(EDK_SOURCE_DATE_EPOCH)

One caution about use of this feature. Since the value changes every build, if the value is used in compiled code, then the value in the compiled code will break build reproducibility. When building production FW images that must always produce the same binary on every build, there must be a way for the production build environment or production DSC file to set this environment variable to a fixed value.

mdkinney avatar May 29 '25 01:05 mdkinney

I agree with mdkinney's solution:

[BuildOptions] __*_CC_FLAGS = -DBUILD_EPOCH=$(EDK_SOURCE_DATE_EPOCH)

On Linux, I can expose EDK_SOURCE_DATE_EPOCH environment manually before building image:

export EDK_SOURCE_DATE_EPOCH=date +%s

joeyli avatar Jul 07 '25 04:07 joeyli

    # Set EDK_SOURCE_DATE_EPOCH to the current time in seconds at start of build
    os.environ["EDK_SOURCE_DATE_EPOCH"] = str(int(time.time()))

One caution about use of this feature. Since the value changes every build, if the value is used in compiled code, then the value in the compiled code will break build reproducibility. When building production FW images that must always produce the same binary on every build, there must be a way for the production build environment or production DSC file to set this environment variable to a fixed value.

So this implies that the python assignment should be conditional on the environment variable being unassigned, right?

ardbiesheuvel avatar Jul 07 '25 23:07 ardbiesheuvel