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

Update mktime header declaration to match upstream LLVM libc. #1972

Closed
wants to merge 1 commit into from

Conversation

Prabhuk
Copy link

@Prabhuk Prabhuk commented Oct 3, 2024

Fixes:#1971

@armandomontanez
Copy link
Contributor

I'm guessing this isn't backwards-compatible with older toolchain distributions?

@kilograham
Copy link
Contributor

.. which it needs to be

@Prabhuk
Copy link
Author

Prabhuk commented Oct 4, 2024

Thanks for bringing up the compatibility question. I should've added the testing info to the PR description.
The change is compatible with the old toolchain revision.

I was able to use the current Clang toolchain revision - e894df6392beea3723627329009f3e6d51d16f47 against pico-sdk master branch with this PR's commit cherry-picked. The bazel build was completed successfully.

I believe adding a specifier in redeclaration is not considered a failure in c++. But I may have to defer this to a language lawyer.

@petrhosek
Copy link
Contributor

mktime is now included in LLVM libc time.h header, so the correct solution here is to delete that declaration altogether, but we also need to bump the toolchain version in MODULE.bazel.

@petrhosek
Copy link
Contributor

I opened #1976 which updates the toolchain.

@Prabhuk
Copy link
Author

Prabhuk commented Oct 7, 2024

mktime is now included in LLVM libc time.h header, so the correct solution here is to delete that declaration altogether, but we also need to bump the toolchain version in MODULE.bazel.

That makes sense. @armandomontanez Will there ever be a case in Pigweed presubmits where the patches landed in the "develop" branch here be built with the toolchain revision mentioned in Pigweed's bazel source files?

@armandomontanez
Copy link
Contributor

Not automatically. For now, we'll be manually revving the required version of the Pico SDK. There may come a day where we do builds against HEAD of develop to learn about breakages sooner, but that's a ways out.

@Prabhuk
Copy link
Author

Prabhuk commented Oct 8, 2024

Not automatically. For now, we'll be manually revving the required version of the Pico SDK. There may come a day where we do builds against HEAD of develop to learn about breakages sooner, but that's a ways out.

Great! I'll close this PR. I'm assuming when we update the toolchain revision in Pigweed, we will update the PicoSDK revision which includes #1976 to get the presubmits to pass.

@Prabhuk Prabhuk closed this Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants