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

Fix build on alpha and x32 architectures #634

Closed
wants to merge 2 commits into from

Conversation

gibmat
Copy link
Contributor

@gibmat gibmat commented Apr 12, 2024

Fix compile errors on alpha and x32 architectures, as seen on the Debian builders.

Signed-off-by: Mathias Gibbens <[email protected]>
#elif defined(__x86_64__) && defined(__ILP32__)
ErrMsgPrintf(errmsg,
"unsupported file system: %llx",
fs_info.f_type);
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this part be fixed by casting fs_info.f_type to a sufficiently wide type instead? Would prefer to avoid the ifdefs if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From statfs(2):

The __fsword_t type used for various fields in the statfs structure definition is a glibc internal type, not intended for public use. This leaves the programmer in a bit of a conundrum when trying to copy or compare these fields to local variables in a program. Using unsigned int for such variables suffices on most systems.

Casting to an unsigned int or unsigned long should work; I copied the ifdefs since there was already one there for s390x.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I hadn't noticed the existing ifdef -- we should remove that too! (No need to do it in this PR.) Casting to unsigned long would be my preferred approach here.

src/tracing.c Show resolved Hide resolved
Copy link

codecov bot commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 81.20%. Comparing base (2b3d6cf) to head (3822a6d).
Report is 1 commits behind head on master.

Files Patch % Lines
src/raft/uv_os.c 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #634      +/-   ##
==========================================
- Coverage   81.21%   81.20%   -0.02%     
==========================================
  Files         192      192              
  Lines       27061    27062       +1     
  Branches     4943     4975      +32     
==========================================
- Hits        21978    21976       -2     
- Misses       3445     3488      +43     
+ Partials     1638     1598      -40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cole-miller
Copy link
Contributor

We also need to you sign the CLA before we can merge this -- sorry for the inconvenience.

Signed-off-by: Mathias Gibbens <[email protected]>
@gibmat gibmat force-pushed the fix-archs-compile branch from 6dfc04d to 3822a6d Compare April 18, 2024 22:55
@gibmat
Copy link
Contributor Author

gibmat commented Apr 18, 2024

We also need to you sign the CLA before we can merge this -- sorry for the inconvenience.

There's nothing mentioning that in the repo (for example, in README, LICENSE, or CONTRIBUTING). 😞 I'm not a fan of CLAs, especially for Debian packaging work which is contributed back under a DFSG-compatible license, rendering the CLA unnecessary.

@cole-miller
Copy link
Contributor

There's nothing mentioning that in the repo (for example, in README, LICENSE, or CONTRIBUTING).

This is a very good point! I'll fix it.

I'm not a fan of CLAs, especially for Debian packaging work which is contributed back under a DFSG-compatible license, rendering the CLA unnecessary.

You have my genuine sympathy here, but we can't do anything about the CLA requirement. If you'd prefer not to sign it then with your blessing I can submit and merge an equivalent PR myself. In either case, I appreciate you contributing this fix!

@gibmat
Copy link
Contributor Author

gibmat commented Apr 19, 2024

I'm not a fan of CLAs, especially for Debian packaging work which is contributed back under a DFSG-compatible license, rendering the CLA unnecessary.

You have my genuine sympathy here, but we can't do anything about the CLA requirement. If you'd prefer not to sign it then with your blessing I can submit and merge an equivalent PR myself. In either case, I appreciate you contributing this fix!

Sure, I'm fine with you making an equivalent PR and then closing this one. That's much preferred versus carrying an additional patch for Debian.

@cole-miller
Copy link
Contributor

Closing as superseded by #640

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.

2 participants