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

Checkout PyNE submodule as part of CMake #734

Merged
merged 7 commits into from
Mar 25, 2021

Conversation

pshriwise
Copy link
Member

Description

Adds an option (default ON) to checkout the PyNE submodule as part of the CMake configuration step.

Motivation and Context

The addition of this submodule in DAGMC makes updates of PyNE in DAGMC more elegant, but it also requires an extra manual step in DAGMC installation (git submodule update --init). There are many build scripts/CI scripts out there that don't include this step and will break as a result.

Changes

This PR incorporates the submodule initialization and update into the CMake step, meaning that no external build scripts will need to change due to the use of PyNE as a submodule.

Behavior

As mentioned above, a section has been added to CMakeLists.txt that will checkout the PyNE submodule automatically. If git is found on the system, the option GIT_SUBMODULE is added with a default value of ON. This option can be turned off for development with new versions of PyNE in DAGMC (another benefit of using the submodule as opposed to the previous approach). If git is not available, then this option will not appear in the CMake configuration and we will expect that the PyNE submodule is already present in the DAGMC source.

@pshriwise pshriwise requested a review from bam241 March 24, 2021 21:43
@pshriwise pshriwise force-pushed the pyne_submodule_cmake branch from 5aaee81 to 22b0035 Compare March 24, 2021 21:50
@gonuke
Copy link
Member

gonuke commented Mar 24, 2021

Did the command-line change for newer version of git? The 18.04 image is using git 2.17.1 and the 16.04 image is using git 2.7.4. The former fails on the submodule and the latter is successful.

@pshriwise
Copy link
Member Author

Did the command-line change for newer version of git? The 18.04 image is using git 2.17.1 and the 16.04 image is using git 2.7.4. The former fails on the submodule and the latter is successful.

Huh, no I don't think it has. I'll dig into it right now.

@pshriwise pshriwise force-pushed the pyne_submodule_cmake branch from badc5d8 to db1b907 Compare March 24, 2021 23:43
@bam241
Copy link
Member

bam241 commented Mar 24, 2021

I think you need a --recursive:

https://cliutils.gitlab.io/modern-cmake/chapters/projects/submodule.html

@pshriwise pshriwise force-pushed the pyne_submodule_cmake branch from db1b907 to 8aad1e9 Compare March 24, 2021 23:49
@pshriwise pshriwise force-pushed the pyne_submodule_cmake branch from 8aad1e9 to 04b4672 Compare March 24, 2021 23:52
@pshriwise
Copy link
Member Author

pshriwise commented Mar 24, 2021

I think you need a --recursive:

https://cliutils.gitlab.io/modern-cmake/chapters/projects/submodule.html

--recursive is already part of the command in CMakeLists.txt. (Though it doesn't appear in the error message. I'll update that.) At any rate, this shouldn't have an effect though as there are no submodules in PyNE to checkout.

Thanks for putting that reference in here btw. That's where this pattern came from.

@bam241
Copy link
Member

bam241 commented Mar 25, 2021

I believe, the git error is: error: pathspec '36' did not match any file(s) known to git.
Do you have any idea how git try to get 36 from our repo or PyNE ?

@gonuke
Copy link
Member

gonuke commented Mar 25, 2021

I saw ‘4’ as the failing pathspec before on the first failure!?!

@gonuke
Copy link
Member

gonuke commented Mar 25, 2021

The strange thing is that it works fine on 16.04

@pshriwise
Copy link
Member Author

I saw ‘4’ as the failing pathspec before on the first failure!?!

Right! It's pretty strange. It seems like something extra is getting added to the command.

@pshriwise pshriwise force-pushed the pyne_submodule_cmake branch 7 times, most recently from 9e8e4ee to d817f7f Compare March 25, 2021 02:07
@pshriwise
Copy link
Member Author

pshriwise commented Mar 25, 2021

Installed a newer version of CMake in CI to get the exact command being run. Not super helpful...

'bash' '-c' '/usr/bin/git submodule update --init --recursive'
error: pathspec '36' did not match any file(s) known to git.

@pshriwise pshriwise force-pushed the pyne_submodule_cmake branch 2 times, most recently from c4ddd68 to f9754de Compare March 25, 2021 04:44
@pshriwise pshriwise force-pushed the pyne_submodule_cmake branch from f9754de to 3f0e4c7 Compare March 25, 2021 06:11
@pshriwise
Copy link
Member Author

pshriwise commented Mar 25, 2021

Wow. I can hardly believe what the problem was. Apparently having the jobs environment variable messes with git submodule commands (maybe others?) on Ubuntu >= 18.04. I tried the following on my DAGMC repo locally (20.04):

jobs=4 git submodule update --init --recursive

And it fails with the same error. The pathspec showing up in CI was the value of the jobs variable. I tried searching to see if this is intended behavior for git, but I couldn't find anything. I wonder if it's worth reporting...

The fix I've added in 3f0e4c7 is just temporary. I think a better long-term solution is to update the jobs variable we set to ci_jobs. This means updating the docker files I think. I can get that process started tomorrow if you're on board with that plan, @gonuke @bam241.

@pshriwise
Copy link
Member Author

Though now that I say that... if we find that updating the docker images will take some time I wouldn't mind getting this solution in sooner as it's holding up some downstream progress elsewhere.

@gonuke
Copy link
Member

gonuke commented Mar 25, 2021

Wild!!!

check out this file (search for "jobs") from the official git repo:

https://github.com/git/git/blob/3a0b884caba2752da0af626fb2de7d597c844e8b/git-submodule.sh

@gonuke
Copy link
Member

gonuke commented Mar 25, 2021

I think we should also remove the git submodule command from .circleci/config.yml

@gonuke
Copy link
Member

gonuke commented Mar 25, 2021

I'm working on rebuilding docker images now, by the way.

@gonuke
Copy link
Member

gonuke commented Mar 25, 2021

Thanks @pshriwise

@gonuke gonuke merged commit 1a8fb79 into svalinn:develop Mar 25, 2021
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