Skip to content

Conversation

@janvorli
Copy link
Member

@janvorli janvorli commented Apr 8, 2021

Port #50105 to Release/5.0

Alpine 3.13 is the first version of Alpine Linux that uses 64 bit time_t
on both 64 and 32 bit platforms. That breaks the build as we assumed
that 32 bit platforms use always 32 bit time_t.

This change fixes it by making the PAL time_t type always 64 bit.
Everything still works fine on non-Alpine ARM / x86 Linuxes, since
it only changes the time_t type size outside of PAL.

Customer Impact

Without this change, we cannot build MUSL ARM runtime on Alpine >= 3.13 and binaries built on Alpine < 3.13 don't work on 3.13+. We want to provide MUSL ARM Alpine 3.13 docker images for 5.0.

Testing

CoreCLR and libraries tests

Risk

Low, PAL defined time_t is used only to set timestamps in pewriter and ilasm and to name core dump in the createdump tool.

Regression

No

* Fix Alpine 3.13 ARM build

Alpine 3.13 is the first version of Alpine Linux that uses 64 bit time_t
on both 64 and 32 bit platforms. That breaks the build as we assumed
that 32 bit platforms use always 32 bit time_t.

This change fixes it by making the PAL time_t type always 64 bit.
Everything still works fine on non-Alpine ARM / x86 Linuxes, since
it only changes the time_t type size outside of PAL.

* Fix case when time function gets NULL argument
@janvorli janvorli added Servicing-consider Issue for next servicing release review area-PAL-coreclr labels Apr 8, 2021
@janvorli janvorli added this to the 5.0.x milestone Apr 8, 2021
@janvorli janvorli requested a review from jkotas April 8, 2021 17:47
@janvorli janvorli self-assigned this Apr 8, 2021
@janvorli
Copy link
Member Author

janvorli commented Apr 8, 2021

cc: @mthalman

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. Please get a cr and we can take for consideration.

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 13, 2021
@leecow leecow modified the milestones: 5.0.x, 5.0.6 Apr 13, 2021
@mmitche mmitche merged commit 4d894f4 into dotnet:release/5.0 Apr 15, 2021
@mmitche
Copy link
Member

mmitche commented Apr 15, 2021

@jeffschwMSFT
Copy link
Member

We have another Alpine 3/13 build fix coming #51188.

@mangod9
Copy link
Member

mangod9 commented Apr 15, 2021

@mmitche the iOS failures look unrelated: embedded dylibs/frameworks only run on iOS 8 or later. Was this the only change in the build?

@mmitche
Copy link
Member

mmitche commented Apr 15, 2021

As far as I know, yes.

@mangod9
Copy link
Member

mangod9 commented Apr 15, 2021

any known compiler or linker change for iOS builds, since its also complaining for other libs which dont appear to be touched. Perhaps someone from Mono can help decipher whether the change to src/libraries/Native/Unix/System.Native/pal_random.c is really causing the break?

@jeffschwMSFT
Copy link
Member

cc @marek-safar @steveisok

@steveisok
Copy link
Member

@akoeplinger was this related to your change?

@janvorli
Copy link
Member Author

The error is
ld : error : embedded dylibs/frameworks only run on iOS 8 or later

@akoeplinger
Copy link
Member

akoeplinger commented Apr 15, 2021

I looked at the commits in release/5.0 and this just suddenly started appearing on a totally unrelated commit a few days ago.

My guess is AzDO updated Xcode to a newer version on the hosted pool Macs which causes this.

Given that we don't support iOS on 5.0 the fastest fix would be to simply disable it there.

(I'm not at my computer right now so won't be able to do that in the next few hours)

@mangod9
Copy link
Member

mangod9 commented Apr 16, 2021

checking in whether the iOS issue is now resolved?

@lukas-lansky
Copy link
Contributor

It doesn't seem so. @akoeplinger, is #51481 what you had in mind?

@ghost ghost locked as resolved and limited conversation to collaborators May 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-PAL-coreclr Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants