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 Mac Build + Update CI. #230

Conversation

CrossR
Copy link
Contributor

@CrossR CrossR commented Nov 1, 2024

Few small changes to make the Mac build happy:

  • Fix some unused variables.
  • PyTorch/LibTorch on Mac doesn't seem to like the use of long, so I've swapped to int64_t there....I've been using this locally on mac for the past 12 months or so, but I (and ideally someone else) need to test this on Linux.
  • Link LArDLContent against LArContent, to access the helpers etc. Again, I've used this on Mac for a while, but would be good to test on Linux.

To help prevent this again, I've also bumped the CI to more modern versions (GCC 12 to match LArSoft v10_00_X, and the latest Clang in Ubuntu 24.04), as well as fixing a few other bits in the CI (i.e. bumping the tools we use in the actions to their latest versions, to prevent some deprecation warnings).

The ROOT and LibTorch versions used here should match the versions used by LArSoft V10_00_02.

Whilst testing this in my local branch, I found that when jumping to the newer compiler versions, I got even more "set but not used variable" warnings, so I've fixed them too.

There is maybe also a question of if we want to bother adding a Mac config to the CI....that would catch these errors quickly, but would be an extra thing running each time (and make the CI config a touch more complicated to support picking the right versions of ROOT / LibTorch, setup steps etcetc).

clang-format-version: '13'
clang-format-version: '18'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't actually actively used...if we want to, we could get the whole code base under clang-format compliance, then enable this on PRs.

@CrossR
Copy link
Contributor Author

CrossR commented Nov 1, 2024

(PR was done purposely against main here, since I don't believe the CI runs against PRs to other branches in its current form....we could look at changing that to remove an extra step for reviewers? I can change the target after the CI runs at least, I just want to verify everything is happy.)

@AndyChappell AndyChappell changed the base branch from master to feature/larpandoracontent_v04_11_01 November 5, 2024 13:00
@AndyChappell AndyChappell merged commit f6c118d into PandoraPFA:feature/larpandoracontent_v04_11_01 Nov 5, 2024
8 checks passed
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