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

Use GNUInstallDirs to fix installation problems on mac #957

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

pbrady
Copy link
Collaborator

@pbrady pbrady commented Oct 13, 2023

PR Summary

A make install on my mac generated the following error:

     479    -- Installing: /parthenon/.
  >> 480    CMake Error at src/cmake_install.cmake:57 (file):
     481      file INSTALL cannot make directory "/parthenon/.": No such file or
     482      directory.
     483    Call Stack (most recent call first):
     484      cmake_install.cmake:42 (include)
     485
     486
  >> 487    make: *** [Makefile:103: install] Error 1

This PR uses GNUInstallDirs to ensure things are defined. The installed directory structure looks correct but I've never installed it before so...

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Oh! This bit me in a previous project and I forgot about it. Good catch! I think this is a sufficiently trivial fix we don't need changelog, etc...

P.S., We often work on branches in parthenon, rather than forks. Would you like push access so you can make those?

@pbrady
Copy link
Collaborator Author

pbrady commented Oct 13, 2023

Thanks! Push access would be great.

@Yurlungur Yurlungur enabled auto-merge October 13, 2023 17:15
@Yurlungur
Copy link
Collaborator

Done

@pbrady
Copy link
Collaborator Author

pbrady commented Oct 16, 2023

Is the CI failure due to the fork workflow or just some random error?

@Yurlungur
Copy link
Collaborator

Looks to me like the CI machine had some internet connection issue:

     for file: [/__w/parthenon/parthenon/tst/regression/gold_standard/parthenon_regression_gold_v19.tgz]
      expected hash: [e1d1a06b9cf9b761d42d0b6b241056ac75658db90138b6b867b1ca7ead4308af4f980285af092b40aee1dbbfb68b4e8cb15efcc9b83d7930c18bf992ae95c729]
        actual hash: [cf83e1357eefb8bdf1542850d66d8007d620e4050b5715dc83f4a921d36ce9ce47d0d13c5d85f2b0ff8318d2877eec2f63b931bd47417a81a538327af927da3e]
             status: [52;"Server returned nothing (no headers, no **data)"]**

If this persists, you might try moving from a fork to a branch. It's possible the CI machine doesn't like running forks. (I vaguely remember hitting that previously)

@pbrady pbrady disabled auto-merge October 16, 2023 18:52
@pbrady pbrady force-pushed the mac-install-fix branch 2 times, most recently from d56b502 to d5896fe Compare October 16, 2023 20:33
@pgrete
Copy link
Collaborator

pgrete commented Oct 18, 2023

The CI error might have been related to a network outage here (in addition to coming from a fork).

I had a look at the code and do not really understand what the issue was and why the proposed changes fix it.
Is there any more documentation (or similar).
I'm mostly worried about potential side effects on various platforms/software environments (that we naturally do not cover with our existing regression tests).

@pbrady pbrady marked this pull request as draft October 18, 2023 14:40
@pbrady
Copy link
Collaborator Author

pbrady commented Oct 18, 2023

The install part was relying on CMAKE_INSTALL_... variables that are not necessarily set by default. Adding the include(GNUInstallDirs) sets all the CMAKE_INSTALL_... variables to the "standard" definitions. I disabled the auto-merge that @Yurlungur had set due to my not understanding how the headers were consumed by other projects (riot) and encountering some errors.

I'll remove the draft status after I sort things out.

@pbrady pbrady marked this pull request as ready for review October 20, 2023 21:15
@pbrady
Copy link
Collaborator Author

pbrady commented Oct 20, 2023

This is ready. I've tested out the installed parthenon with riot and that works so this is probably doing the right thing.

@Yurlungur Yurlungur enabled auto-merge October 22, 2023 19:20
@Yurlungur
Copy link
Collaborator

@pgrete @pdmullen @jdolence @BenWibking @lroberts36 @forrestglines can we get one more approval?

@pdmullen
Copy link
Collaborator

I would be happy to approve this (hence instantiating the auto-merge), but @pgrete raised some concerns above, so I think it best that he takes a look too.

Copy link
Collaborator

@pgrete pgrete 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. I just tested that at least locally this works as expected and it does!
I'm approving (tough I think it'd be nice to also add it to the changelog so that people like @anku94 who use it as library know that there was a change related how the install works/behaves).

@pgrete pgrete disabled auto-merge November 10, 2023 14:12
@pgrete pgrete enabled auto-merge (squash) November 10, 2023 14:12
@pgrete
Copy link
Collaborator

pgrete commented Nov 10, 2023

Looks like I had to re-enable auto-merge to trigger the extended CI tests.

@pgrete
Copy link
Collaborator

pgrete commented Nov 10, 2023

Style check still doesn't seem to trigger for forks. As the extended test pass I'm manually pulling the trigger.

@pgrete pgrete disabled auto-merge November 10, 2023 15:27
@pgrete pgrete enabled auto-merge (squash) November 10, 2023 15:28
@pgrete pgrete disabled auto-merge November 10, 2023 15:28
@pgrete pgrete merged commit a8cea59 into parthenon-hpc-lab:develop Nov 10, 2023
34 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.

4 participants