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

openPMD-api: new CMake Control (tests) #312

Merged
merged 3 commits into from
Jan 15, 2021
Merged

openPMD-api: new CMake Control (tests) #312

merged 3 commits into from
Jan 15, 2021

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Jan 12, 2021 •
edited
Loading

Use the latest commit in openPMD-api (dev branch post 0.13.1, PR openPMD/openPMD-api#897) that adds control for per-project build options for tests.

This saves us from building and running openPMD-api tests in the HiPACE superbuild.

Fix #307

  • Small enough (< few 100s of lines), otherwise it should probably be split into smaller PRs
  • Tested (describe the tests in the PR description)
  • Runs on GPU (basic: the code compiles and run well with the new module)
  • Contains an automated test (checksum and/or comparison with theory)
  • Documented: all elements (classes and their members, functions, namespaces, etc.) are documented
  • Constified (All that can be const is const)
  • Code is clean (no unwanted comments, )
  • Style and code conventions are respected at the bottom of https://github.com/Hi-PACE/hipace
  • Proper label and GitHub project, if applicable

@ax3l
openPMD-api: new CMake Control (tests)
08190bc 
Use the latest commit in openPMD-api (dev branch post 0.13.1) that
adds control for per-project build options for tests.

This saves us from building and running openPMD-api tests in the
HiPACE superbuild.
@ax3l ax3l added the component: diagnostics About any types of diagnostics label Jan 12, 2021
@ax3l ax3l requested a review from MaxThevenet January 12, 2021 22:26
@ax3l ax3l added build CMake, compilation, etc. CI Continuous integration, checksum and analysis tests, GitHub Actions, etc. bug Something isn't working labels Jan 12, 2021
MaxThevenet
MaxThevenet reviewed Jan 13, 2021
View reviewed changes
Copy link
Member

@MaxThevenet MaxThevenet 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, thanks! See one comment below.

cmake/dependencies/openPMD.cmake Outdated
@@ -62,7 +62,7 @@ if(HiPACE_OPENPMD)
set(HiPACE_openpmd_repo "https://github.com/openPMD/openPMD-api.git"
CACHE STRING
"Repository URI to pull and build openPMD-api from if(HiPACE_openpmd_internal)")
set(HiPACE_openpmd_branch "0.13.1"
set(HiPACE_openpmd_branch "a9022ee30fe640a5ca1d92c30d0658cf2bfebec6"
Copy link
Member

Choose a reason for hiding this comment

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

is there a plan to go back to a release version here? If so, should we wait before merging this PR?

Copy link
Member Author

@ax3l ax3l Jan 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, the plan is to use this until a new major release comes out.
It's still reproducible and fine and allows us to move fast.

This change is a bit invasive, so I am not 100% sure I can drop it in a 0.13.2 patch release (although I try to make it backwards compatible). Will check out, latest it will be in 0.14.0 :)

@ax3l ax3l changed the title openPMD-api: new CMake Control (tests) [WIP] openPMD-api: new CMake Control (tests) Jan 14, 2021
@ax3l
Copy link
Member Author

ax3l commented Jan 14, 2021

This actually needs another merge, forgot some options 😅

openPMD/openPMD-api#900

@MaxThevenet
Copy link
Member

Ok then I'm fine with keeping the commit hash. Just to recap, is this what you propose?

  1. Wait until CMake: Fix New Prefixes openPMD/openPMD-api#900 is merged
  2. Commit your suggestion
  3. merge this PR
  4. Replace HiPACE_openpmd_branch "a9022ee30fe640a5ca1d92c30d0658cf2bfebec6" with a proper version when all options are supported

That's fine with me.

@ax3l
New commit
0089f65 
Includes follow-up PR 900 for the new controls
@ax3l
Copy link
Member Author

ax3l commented Jan 14, 2021 •
edited
Loading

Exactly, 4. would be in a couple of weeks/month, once the next release comes out that includes this.

Release tags are in the end a named alias for a commit, we now sit between two tags.
A couple weeks back (and for AMReX), we just pull(ed) the named HEAD of the dev branch, which is a "dynamic" alias for a commit. We rather use tags/commits now, since it does not break downstream (WarpX/HiPACE) CI if something breaking is introduced in (openPMD-api) development.

@ax3l ax3l changed the title [WIP] openPMD-api: new CMake Control (tests) openPMD-api: new CMake Control (tests) Jan 14, 2021
@MaxThevenet @ax3l
Apply suggestions from code review
cc4c4ca 
Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
MaxThevenet
MaxThevenet approved these changes Jan 15, 2021
View reviewed changes
Copy link
Member

@MaxThevenet MaxThevenet left a comment

Choose a reason for hiding this comment

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

Sounds good, thanks!

@MaxThevenet MaxThevenet merged commit d1a9855 into development Jan 15, 2021
@MaxThevenet MaxThevenet deleted the topic-openPMDapiDevNewBldOpt branch January 15, 2021 13:51
@ax3l
Copy link
Member Author

ax3l commented Feb 11, 2021 •
edited
Loading

There will be a follow-up to this once openPMD/openPMD-api#930 is merged in
The openPMD_BUILD_SHARED_LIBS control was not yet fully working and did still build shared libs if the overall project was building as shared as well (we don't in HiPACE++ by default).

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

@MaxThevenet MaxThevenet MaxThevenet approved these changes

Assignees
No one assigned
Labels
bug Something isn't working build CMake, compilation, etc. CI Continuous integration, checksum and analysis tests, GitHub Actions, etc. component: diagnostics About any types of diagnostics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Avoid running openPMD CI '-.-
2 participants
@ax3l @MaxThevenet