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

macOS Fixes, main branch (2024.10.26.) #755

Merged
merged 3 commits into from
Oct 26, 2024

Conversation

krasznaa
Copy link
Member

Triggered by the report from @CrossR and Fabrice, these are things that I needed to fix to make the project (without any GPU support of course...) build on macOS 15.0.1 with Xcode 16.0.

The biggest one being that traccc_benchmarks_common was set up very incorrectly so far. Yet, the archival application (ar) on Linux did not complain about this for some reason. Apparently on Linux it's okay to create a static library with absolutely no content. But on macOS it clearly isn't. 🤔

The rest are some conversion issues and a missing include.

For some reason the misconfiguration only showed up with
macOS's archiver. :-/
@krasznaa krasznaa added bug Something isn't working build This relates to the build system cpu Changes related to CPU code labels Oct 26, 2024
@@ -1,6 +1,6 @@
/** TRACCC library, part of the ACTS project (R&D line)
*
* (c) 2022 CERN for the benefit of the ACTS project
* (c) 2022-2024 CERN for the benefit of the ACTS project
Copy link
Member

Choose a reason for hiding this comment

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

BTW could consider following ACTS' change to only list the initial year instead of updating files.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm actually partial to the convention we have in the ATLAS offline code, where we show the last modification date of each file. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Sure. We had a discussion about this, and ultimately decided not to duplicate modification date (that's what blame is for) since it avoids it getting out of date, and the actual year being legally meaningless anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah strong support for the ACTS style of not updating these; avoids a lot of pain in the rear and it has zero legal meaning.

Copy link
Member Author

Choose a reason for hiding this comment

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

You guys are making it sound like it's a black and white issue. But it's very much a shade of grey in my mind.

For instance, what about when we re-design some code. Let's say we break up a class into two separate classes, with "new" names. What would be the appropriate date on the files of the new classes? Would one use the current year? The year when the original class was written?

In the scheme that we've been using so far in the R&D projects, the new files would reflect both that their code came from an earlier time, and that that code has now moved into a new place.

Paul is correct, even we discussed about this in the past. There can be different legitimate conventions for this. But given that the convention in this repository has not been the one lobbied for now, if we want to switch to that convention, we need to do it clearly.

I.e. somebody should open a PR, and update all files accordingly. 🤔 And update the contribution guide. (I'm just kidding. I meant write a contribution guide...)

@paulgessinger
Copy link
Member

The sonar issue is just complaining about pseudo random numbers as usual.

Only in Release mode, to not bloat the CI time too much.
@krasznaa krasznaa force-pushed the macOSFixes-main-20241026 branch from 6732f3f to 6470016 Compare October 26, 2024 13:15
Copy link

@krasznaa
Copy link
Member Author

Decided to add a CI test on macOS already as part of this PR. Tried to make it as simple as possible.

@paulgessinger, what do you think?

Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@stephenswat stephenswat merged commit c88c85f into acts-project:main Oct 26, 2024
24 checks passed
@krasznaa krasznaa deleted the macOSFixes-main-20241026 branch October 28, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working build This relates to the build system cpu Changes related to CPU code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants