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

Added untracked headers in cmake #228

Merged
merged 2 commits into from
Feb 3, 2023
Merged

Conversation

cmauney
Copy link
Contributor

@cmauney cmauney commented Feb 3, 2023

PR Summary

Addressing #227 . singularity-eos/CMakeLists.txt updated to include all header files in code.

This could be an impetus to automate source file discovery in the build, but my impression is GLOB and similar commands are not recommended. This could be an outdated mode of thought, tho.

PR Checklist

  • N/A Adds a test for any bugs fixed. Adds tests for new features.
  • N/A Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file
  • N/A If preparing for a new release, update the version in cmake.

@mauneyc-LANL mauneyc-LANL self-assigned this Feb 3, 2023
@mauneyc-LANL
Copy link
Collaborator

@dholladay00

@dholladay00
Copy link
Collaborator

Have you tested to make sure we're not missing anything else?

@mauneyc-LANL
Copy link
Collaborator

Have you tested to make sure we're not missing anything else?

Just find on command line

@dholladay00
Copy link
Collaborator

Would it be possible to add building against an installed singularity-eos in the CI @mauneyc-LANL ?

@mauneyc-LANL
Copy link
Collaborator

Would it be possible to add building against an installed singularity-eos in the CI @mauneyc-LANL ?

There's already an install test run on github CI. It just checks for one header, so that's likely why it wasn't caught.

@Yurlungur
Copy link
Collaborator

Thanks for the quick fix @mauneyc-LANL . I suppose we could add an install test that checks for more headers but I'm not sure what the size of that list ends up.

@Yurlungur Yurlungur merged commit fa32862 into main Feb 3, 2023
@Yurlungur Yurlungur deleted the mauneyc/fix/includes_fix branch February 3, 2023 18:50
@mauneyc-LANL mauneyc-LANL mentioned this pull request Feb 3, 2023
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.

4 participants