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 submodules for included libraries (oasis, json-fortran, datetime-fortran)? #29

Closed
aekiss opened this issue Sep 11, 2019 · 9 comments
Closed
Assignees

Comments

@aekiss
Copy link
Contributor

@aekiss aekiss commented Sep 11, 2019

I'm wondering why we don't incorporate these into the libaccessom2 repo as submodules?
https://github.com/COSIMA/oasis3-mct
https://github.com/jacobwilliams/json-fortran
https://github.com/nicjhan/datetime-fortran

At present these are git cloned by cmake (see ExternalProject_Add entries in CMakeLists.txt) which seems unnecessarily obscure, as it means we don't see the full codebase until cmake is run.

I'd prefer these to be submodules, so a git clone --recursive (either of libaccessom2 or access-om2) will deliver all the source code, with no cmake dependency.

The present arrangement makes it too easy to overlook these libraries when searching the codebase (e.g. #24), especially if using an offline clone of the code (e.g. on my laptop, where I wouldn't normally run cmake).

@aekiss aekiss changed the title use submodules for included libraries (oasis, json-fortan, datetime-fortran)? use submodules for included libraries (oasis, json-fortran, datetime-fortran)? Sep 12, 2019
@aekiss aekiss mentioned this issue Dec 2, 2019
@nichannah

This comment has been minimized.

Copy link
Contributor

@nichannah nichannah commented Jan 10, 2020

I think that the cmake approach is simpler than the git submodule approach (e.g. being able to see the repo url and hash without git commands).

However since we already use submodules it is introducing more complexity and more stuff to know.

So I think we do this issue and then if we decide we don't like git submodules later then switch everything over to the cmake approach. i.e. just have one approach or the other not both.

@nichannah nichannah self-assigned this Jan 10, 2020
@aekiss

This comment has been minimized.

Copy link
Contributor Author

@aekiss aekiss commented Jan 10, 2020

OK, sounds good

@nichannah

This comment has been minimized.

Copy link
Contributor

@nichannah nichannah commented Jan 10, 2020

This has been merged into libaccessom2/dev branch

@nichannah nichannah closed this Jan 10, 2020
@aekiss

This comment has been minimized.

Copy link
Contributor Author

@aekiss aekiss commented Jan 15, 2020

just noting here that this is commit 794284b
(the commit message had the wrong issue number)

@aekiss

This comment has been minimized.

Copy link
Contributor Author

@aekiss aekiss commented Jan 15, 2020

@nichannah this commit will change to a much newer version of json-fortran. That might be a good thing, or it might break things. If the latter, we should check out the specific commit we want.
794284b#diff-8903239df476d7401cf9e76af0252622

@aekiss aekiss reopened this Jan 15, 2020
@aekiss

This comment has been minimized.

Copy link
Contributor Author

@aekiss aekiss commented Jan 15, 2020

Perhaps we could use our own fork of json-fortran (as you've done with oasis3-mct and datetime-fortran) rather than referring directly to the originals? This doesn't really help with the version issue, but at least means we have a copy of the repo in case it is removed.

@aekiss

This comment has been minimized.

Copy link
Contributor Author

@aekiss aekiss commented Jan 15, 2020

It would be better to use https://github.com/COSIMA/datetime-fortran rather than https://github.com/nicjhan/datetime-fortran so more of us have edit access.

There have been a few minor-looking updates to https://github.com/wavebitscientific/datetime-fortran/commits/master - might be worth updating our fork?

@nichannah

This comment has been minimized.

Copy link
Contributor

@nichannah nichannah commented Jan 19, 2020

OK, good suggestions.

@nichannah

This comment has been minimized.

Copy link
Contributor

@nichannah nichannah commented Jan 24, 2020

This has been merged into dev

@nichannah nichannah closed this Jan 24, 2020
nichannah added a commit that referenced this issue Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.