Skip to content

Conversation

@wdconinc
Copy link
Member

JANA2 is a C++ only project. By default CMake sets LANGUAGES C CXX, which causes JANA2 to search for and require a C compiler.

@nathanwbrei
Copy link
Collaborator

@wdconinc @veprbl The vendored md5 dependency is pretty awkward. It's used internally by JResource and JGeometry but is not part of the install interface at all. So one option is "porting" it to C++, which may be trivial. Any other suggestions?

@wdconinc
Copy link
Member Author

wdconinc commented Apr 1, 2025

Is this a case where a user would specify an md5 hash and you compare with what you get? Or is it entirely internal, and all you need is any hash function (and also std::hash would do)?

@nathanwbrei
Copy link
Collaborator

Is this a case where a user would specify an md5 hash and you compare with what you get? Or is it entirely internal, and all you need is any hash function (and also std::hash would do)?

Sadly it does have to be md5, as is for validating that resource and geometry files were downloaded correctly, and is compared to values already stored in the calibration db, which were also probably generated externally.

I'm mildly annoyed that an md5 implementation is not provided by the standard library, and baffled that everyone on stackoverflow recommends using the openssl implementation. I remember having suffered mightily with broken OpenSSL dependencies post-Heartbleed when I first came to JLab.

@wdconinc
Copy link
Member Author

wdconinc commented Apr 2, 2025

I'm mildly annoyed that an md5 implementation is not provided by the standard library, and baffled that everyone on stackoverflow recommends using the openssl implementation. I remember having suffered mightily with broken OpenSSL dependencies post-Heartbleed when I first came to JLab.

I've seen JLab projects include TMD5.h for no other reason than an md5 hash on a file, but I'll refrain from making that suggestion here... (it's also not thread safe from what I recall, or had some other bug that breaking online analyzer functionality...)

@wdconinc
Copy link
Member Author

wdconinc commented Apr 8, 2025

I'm perfectly fine with marking this as a WONTFIX. It's not a big deal and I just thought that JANA2 only had C++ code in it...

Fix undefined behavior when checking data alignment
@nathanwbrei
Copy link
Collaborator

nathanwbrei commented Apr 24, 2025

I haven't closed this PR as WONTFIX because I kind of like the noise reduction. I also like not mixing C and C++ code because they really are different languages at this point. Here's an attempt at a C++ port of md5 which hopefully fixes the PR.

Do you guys think this is a good idea, or not?

The biggest argument against this PR that I see is that implementation-defined behavior in C is undefined behavior in C++. I'm not sure I trust this code because UBSan won't run along all code paths. Maybe I need a static UB checker?

Or maybe I should just pull in a pre-existing C++ md5 implementation, such as https://github.com/CommanderBubble/MD5

Comment on lines +171 to 172
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Iffy indentation.

@wdconinc
Copy link
Member Author

Looking at the code, are you mostly worried about verifying the branches with different BYTE_ORDER? I don't see a whole lot of unexplored branches (in a quick glance).

@wdconinc wdconinc mentioned this pull request Aug 6, 2025
7 tasks
wdconinc added a commit to eic/eic-spack that referenced this pull request Aug 7, 2025
### Briefly, what does this PR introduce?
Since JANA2 does not explicitly include only `LANGUAGES CXX` in the
CMakeLists.txt (see JeffersonLab/JANA2#419), we
must ensure a C compiler is available.

### What kind of change does this PR introduce?
- [x] Bug fix (issue:
https://eicweb.phy.anl.gov/containers/eic_container/-/jobs/6090505#L3722)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No.

### Does this PR change default behavior?
No.
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