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.

Sign up for GitHub

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

Jump to bottom

Potential fix for Arm Linux testing. #583

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Potential fix for Arm Linux testing. #583

wants to merge 8 commits into from
+2 −2

Conversation

EricEngle-NOAA
Copy link
Contributor

Potential fix for #560

@EricEngle-NOAA
Potential fix for Arm Linux testing.
e7e70ea
@EricEngle-NOAA
Take 2...
d76a0a1 
This time I have commented out shifting bits when writing a
1-byte negative number (signed char).
@EricEngle-NOAA
Take 3
1aa1f74 
Take 2 was successful except for a docs generation error

error: Member IS_ARM_LINUX (macro definition) of file grib2_int.h
is not documented. (warning treated as error, aborting now).
@EricEngle-NOAA
Take 4...
5083750 
Cleaned up with clang-format.
@EricEngle-NOAA EricEngle-NOAA marked this pull request as ready for review January 17, 2025 20:16
edwardhartnett
edwardhartnett reviewed Jan 18, 2025
View reviewed changes
src/grib2_int.h Outdated
@@ -27,6 +27,9 @@

#include "grib2.h"

/** Define is system is Arm Linux. */
#define IS_ARM_LINUX __aarch64__ &&__linux__
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want this define.

Is there a way to detect whatever is happening on ARM and handle it without having platform specific defines?

@EricEngle-NOAA
Copy link
Contributor Author

@edwardhartnett After thinking about this, and looking at the output, it is not writing in accordance with the GRIB2 standard for negative values. So 63116bb despite passing the checks is not correct. Continuing to work on this...

@EricEngle-NOAA
Copy link
Contributor Author

If this is an acceptable PR, can we issue g2c v2.1.1?

@edwardhartnett
Copy link
Contributor

If we can agree on a fix, we can merge it and do a release. But I don't want to reintroduce machine-specific macros to g2c.

So what is the condition on ARM that is being detected here?

@EricEngle-NOAA
Copy link
Contributor Author

@edwardhartnett the most recent change, f51770c, removes architecture/platform specific defines.

In digging deeper, on ARM Linux the default char is unsigned char where on every other platform, including Apple silicon, default char is signed char.

@edwardhartnett
Copy link
Contributor

OK, that's a lot better.

Can we have some testing for this code? Perhaps a test which fails with the old code but succeeds with the new code?

@EricEngle-NOAA
Copy link
Contributor Author

I am bit confused with this. What do you mean by old code? It would not exist. Do you mean forcing a failure in the tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edwardhartnett edwardhartnett edwardhartnett left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@EricEngle-NOAA @edwardhartnett