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

Simplify Housekeeping Process for DAGMC #943

Merged
merged 11 commits into from
Feb 22, 2024

Conversation

ahnaf-tahmid-chowdhury
Copy link
Member

Description

This PR simplifies the housekeeping process for DAGMC by removing the dependency on a Docker container and installing clang-format directly through the workflow.

Related Issues

Housekeeping of DAGMC is very complicated now. And it seems in the docker file, we are just installing clang-format through apt. Also, I noticed the docker publish file is changed and there is no way to rebuild a new docker image for housekeeping.

Changes

  • Removed the use of a Docker container for housekeeping.
  • Installed clang-format directly through the workflow.

@ahnaf-tahmid-chowdhury ahnaf-tahmid-chowdhury changed the title remove using container Simplify Housekeeping Process for DAGMC Feb 20, 2024
@gonuke
Copy link
Member

gonuke commented Feb 20, 2024

perhaps we should update these two files to match what clang-format is generating as part of this PR so that it passes tests

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

This looks good - I thought I had done this already :) but maybe it's lost in my local branches somewhere.

We can perhaps take care of the two formatting inconsistencies in the current code that are causing this to fail right now. I wonder if they are due to slight changes in clang-format's behavior since the docker images was generated.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

One small request

src/geant4/app/include/ExN01UserScoreWriter.hh Outdated Show resolved Hide resolved
gonuke
gonuke previously approved these changes Feb 21, 2024
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Thanks for streaming in this @ahnaf-tahmid-chowdhury

@gonuke
Copy link
Member

gonuke commented Feb 21, 2024

This now has a conflict in the CHANGELOG.rst

@gonuke
Copy link
Member

gonuke commented Feb 22, 2024

It looks like there is an update in CMake version in the new Windows runner... I'm not sure yet if/why that is causing a problem.

@gonuke
Copy link
Member

gonuke commented Feb 22, 2024

We may need to provide more information to our CMake config for MOAB. It may be true that MOAB should improve their custom FindEigen macro, but providing EIGEN3_DIR may be easier

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up @ahnaf-tahmid-chowdhury

@gonuke gonuke merged commit ab4db53 into svalinn:develop Feb 22, 2024
20 checks passed
@ahnaf-tahmid-chowdhury ahnaf-tahmid-chowdhury deleted the housekeeping branch February 22, 2024 11:47
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