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

Add support for building on Ubuntu 24.04 #119

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

jmblixt3
Copy link
Contributor

Add include for cstdint in error.h

Add include for cstdint in error.h
@iamthebot
Copy link

Can this please be merged? This shouldn't break other builds.

Copy link

@iamthebot iamthebot left a comment

Choose a reason for hiding this comment

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

Tested this patch works on Ubuntu 22.04 with GCC 14.1.1.

This is probably going to be required for all newer distros as well.

@rmccorm4
Copy link
Contributor

Hi @jmblixt3 , thanks for the contribution!

@mc-nv @nv-kmcgill53 @fpetrini15 can you take a sanity check look here to make sure this is OK with RHEL, Windows, etc?

@fpetrini15
Copy link
Contributor

fpetrini15 commented Aug 21, 2024

@rmccorm4 I just tested compiling a simple program with this include in our RHEL base and Windows build base containers and didn't encounter any issues.

@nv-kmcgill53
Copy link
Contributor

nv-kmcgill53 commented Aug 21, 2024

My only question is: How was this compiling before? We have multiple references to the stdint types in our code and this wasn't needed for the last 4 years. What changed?

edit: I am fine with the change, just want to know why it's needed

@rmccorm4
Copy link
Contributor

Seems to be something with gcc 14 specifically, it looks like our builds use gcc 11. Similar PR: NVIDIA/DALI#5591

@nv-kmcgill53
Copy link
Contributor

I don't understand why this is a breaking change with a newer version of gcc. That seems like bad design. Approved.

@nv-kmcgill53
Copy link
Contributor

@jmblixt3 Have you filled out the Triton CCLA? I don't see your company or or user in our files. If not, can you please fill this out so we can merge your PR? Thanks!

https://github.com/triton-inference-server/server/blob/main/Triton-CCLA-v1.pdf

@rmccorm4
Copy link
Contributor

@nv-kmcgill53 Seems like some types were exposed in other headers like <string> in older compiler versions (gcc 11), but not in newer ones (gcc14)? See repro:

@jmblixt3
Copy link
Contributor Author

@jmblixt3 Have you filled out the Triton CCLA? I don't see your company or or user in our files. If not, can you please fill this out so we can merge your PR? Thanks!

https://github.com/triton-inference-server/server/blob/main/Triton-CCLA-v1.pdf

Just did it

@nv-kmcgill53
Copy link
Contributor

Thank you for sending in your CLA and contributing to the Triton project. We have accepted it and can merge your PR at our discretion.

@iamthebot
Copy link

I don't understand why this is a breaking change with a newer version of gcc. That seems like bad design. Approved.

Technically this PR is the correct behavior-- std::uint8_t is defined in cstdint and so that header should be included. There are cases where a replacement is desired, for example, and automatic inclusion of cstdint isn't desirable. Newer stdlibs and gcc versions seem to be more strict in that sense.

@nv-kmcgill53 nv-kmcgill53 merged commit 578491f into triton-inference-server:main Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants