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

Cmake updates #1

Merged
merged 6 commits into from
Jun 20, 2020
Merged

Cmake updates #1

merged 6 commits into from
Jun 20, 2020

Conversation

mnlevy1981
Copy link

Scripts in each of the subdirectories of reg_test/ can pass the --cmake argument, and it will launch cmake from bld/cmake_bld and then use the resulting executable (bld/cmake_bld/cvmix_driver) instead of bin/cvmix.

Some missing features:

  1. Ability to clean bld/cmake_bld
  2. Ability to specify compiler (gfortran is hard-coded)
  3. netCDF support
  4. --cmake option in CVMix_tools/run_test_suite.sh

But at least it's a way for us to test the cmake in Travis.

At this point, it's pretty klunky -- no netcdf support, forces compiler to be
gfortran. But it should get cmake tested in travis, which is a good start.
Travis CI provides cmake 3.9.2 (we are using an old linux distribution so the
apt-get versions of gfortran and libnetcdf-dev play nicely together); hoping
there aren't actually features introduced in 3.10 in the system...
@mnlevy1981
Copy link
Author

I set up a pull request in my own fork just to trigger Travis builds, and 1cb7ba2 failed with

$ cd ../reg_tests/Bryan-Lewis/; ./Bryan-Lewis-test.sh --cmake
CMake Error at CMakeLists.txt:1 (cmake_minimum_required):
  CMake 3.10 or higher is required.  You are running version 3.9.2

-- Configuring incomplete, errors occurred!
Build error!
The command "cd ../reg_tests/Bryan-Lewis/; ./Bryan-Lewis-test.sh --cmake" exited with 2.

but it looks like b583d7d is fine

Also modified build.sh and run.sh to follow directions laid out in the README
(created cmake_bin/ for build target, build.sh calls "make install" and run.sh
runs from cmake_bin/bin/)
@mnlevy1981
Copy link
Author

4c61476 also passes Travis tests, so I think this branch is ready to merge and then we can continue discussion on CVMix#88

@qingli411
Copy link

@mnlevy1981 Hi Mike, Just want to make sure that we should first merge these changes to Karsten's master branch then we can proceed to merging these to the main CVMix repo, right?

@mnlevy1981
Copy link
Author

@qingli411 That would be ideal, as it would update CVMix#88 and I could update my review. Alternatively, I could just merge my branch into master and that should also close the PR (and I'd add a comment about additional commits addressing open concerns)

@qingli411
Copy link

OK. I see. I guess it's perhaps cleaner to merge these changes to Karsten's branch, then we can have all the CMake relevant discussions and changes in CVMix#88. @bolding What do you think?

@bolding
Copy link
Owner

bolding commented Jun 19, 2020 via email

@mnlevy1981
Copy link
Author

it's perhaps cleaner to merge these changes to Karsten's branch, then we can have all the CMake relevant discussions and changes in CVMix#88

I concur.

I think he should merge with squash

@bolding if you want me to do a squash merge, could you please merge this PR (without the squash) and then I'll squash-merge your PR?

@bolding
Copy link
Owner

bolding commented Jun 19, 2020 via email

@qingli411
Copy link

@bolding Have you pushed your merged master branch to Github? I didn't see it here...

@mnlevy1981
Copy link
Author

@bolding it looks like you merged your master into my cmake_updates branch, but to update CVMix#88 you need to merge my cmake_updates back into your master (and, as @qingli411 points out, push back to github)

@bolding
Copy link
Owner

bolding commented Jun 20, 2020

I must say I'm confused now.

When we worked on this a month ago I made updates to my repository including all changes requested. So I don't see why I need for a PR. And I did not notice the PR from @mnlevy1981 until @qingli411 informed me about it yesterday. I then followed the instructions by GitHub and did a merge (including solving a conflict in README - misspelling of installation (no a) - actually surprised why it was there in the first place as I had fixed it a while ago).

So - what is the status now?

@bolding bolding merged commit d416bde into bolding:master Jun 20, 2020
@bolding
Copy link
Owner

bolding commented Jun 20, 2020

Instead of using the GitHub web-interface I now did it via the command line - hopefully it has worked out as you want/expect now. At least git log shows fixes by Michael.

Sorry for the confusion - if I'm to blame :-)

@qingli411
Copy link

@bolding Thanks! I think now we have all the changes here merged to CVMix#88. I think @mnlevy1981 created this PR because he'd like to add some more changes. Instead of requesting you to make these additional changes, he did that on his side. Then it is cleaner to just add these changes directly to your branch so that they can be merged into the CVMix main repository using the same PR. Otherwise he will have to create another PR in the CVMix main repository.

@mnlevy1981
Copy link
Author

@bolding

Sorry for the confusion - if I'm to blame :-)

I think I was relying on github notifications to let you know this pull request existed, and maybe also didn't explain everything that I thought I had typed out. So apologies for my role in the hold-up. CVMix#88 has been merged, though - thanks so much for the contribution!

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.

3 participants