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

Replace boutdata and boututils directories with submodules #2196

Merged
merged 8 commits into from
Jan 12, 2021

Conversation

johnomotani
Copy link
Contributor

Get boutdata and boututils from the separate git repos (github.com/boutproject/boutdata and github.com/boutproject/boututils), including them as submodules.

I did check that the current versions of boututils and boutdata in next are identical to the versions in the separate git repos.

Get boutdata and boututils from the separate git repos
(github.com/boutproject/boutdata and github.com/boutproject/boututils),
including them as submodules
@johnomotani
Copy link
Contributor Author

I added symlinks tools/pylib/boutdata -> externalpackages/boutdata/boutdata and tools/pylib/boututils -> externalpackages/boututils/boututils so the changes should be totally backward-compatible, as long as the submodules have been checked out.

@johnomotani johnomotani added the backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 label Jan 3, 2021
bendudson
bendudson previously approved these changes Jan 4, 2021
Copy link
Contributor

@bendudson bendudson left a comment

Choose a reason for hiding this comment

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

Thanks @johnomotani removing this duplication is good.

@johnomotani
Copy link
Contributor Author

I think there's a problem with the automatic version numbering, that was recently added to boututils and boutdata, possibly related to the way I used symlinks. Investigating now...

@johnomotani
Copy link
Contributor Author

When boutproject/boututils#16 and boutproject/boutdata#26 are merged, we can update the submodules and then the tests will pass.

@johnomotani
Copy link
Contributor Author

The Fedora builds are failing. I need to add the Python setuptools_scm package, but I'm not sure how to do that for Fedora. It looks like there is a Fedora package https://src.fedoraproject.org/rpms/python-setuptools_scm. @dschwoerer @ZedThree where/how can I add it? Thanks!

@ZedThree
Copy link
Member

ZedThree commented Jan 5, 2021

I think you should just be able to add it here:

time dnf -y install dnf-plugins-core {petsc,hdf5}-${mpi}-devel /usr/lib/rpm/redhat/redhat-hardened-cc1 python3-h5py

@dschwoerer maintains the bout++ rpm which will bring in other dependencies, so it could be added there too -- that's external and I don't have access to do that.

Another option would be to pip install the packages in that script, after L40. We might actually want to do this for all the CI builds, as it will means less changes when the packages get updated.

Is setuptools_scm now a hard dependency to use the boutdata/boututils submodules in this repo?

@johnomotani
Copy link
Contributor Author

Is setuptools_scm now a hard dependency to use the boutdata/boututils submodules in this repo?

Yes. It's not required when boutdata/boututils are installed, but since we use them straight from the git repos in the the submodules it is required.

@johnomotani
Copy link
Contributor Author

I added python-setuptools_scm to the Fedora build as @ZedThree suggested, and setuptools_scm is now there. Now it complains that git isn't there, and therefore setuptools_scm can't get the version number for boututils. I'm a bit lost now. @dschwoerer what should we be doing with submodules in Fedora?

@ZedThree
Copy link
Member

ZedThree commented Jan 5, 2021

Just adding git to the same line should be sufficient I think? The Fedora build creates a docker image and copies the checked-out source into it, so the image doesn't actually use git.

@johnomotani
Copy link
Contributor Author

The Fedora build creates a docker image and copies the checked-out source into it, so the image doesn't actually use git.

If it does the same for the submodules then I guess git wouldn't actually help, but if we go with the suggestion in #2198 (comment) to avoid a hard requirement on setuptools_scm then we'd get around this issue anyway.

johnomotani and others added 2 commits January 5, 2021 22:36
It is not required any more.
* next: (36 commits)
  Removed bout_runners from repo, and redirected to project site
  Allow test-options-netcdf on Fedora
  Add missing brackets in test-options-netcdf
  Skip test-io when using legacy netCDF interface
  clang-format FCIMap constructor
  Add braces around conditional body
  Pass dy by reference
  Guard OptionsNetCDF test against legacy netcdf
  Cache index offsets in FCI boundary loop
  Invert conditional in FCI boundary calculation
  Convert nested-loop to BOUT_FOR in FCI
  Make local variables const
  Pass dy down into FCIMap
  Remove some unused code from FCI
  Guard against using legacy netCDF for ncxx4 and OptionsNetcdf
  Fix duplicated code when using legacy netCDF
  Set BOUT_HAS_NETCDF for both legacy detection methods
  GHA: Increase test timeout to 6 minutes
  Add expectation of `datadir`, `dump_format` options having been set
  Remove unnecessary logical OR in build_config header
  ...
@johnomotani johnomotani merged commit 7fe0f8d into next Jan 12, 2021
@johnomotani johnomotani deleted the pylib-submodules branch January 12, 2021 11:29
@johnomotani johnomotani mentioned this pull request Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport candidiate Does not break backward compatibility, so can be back-ported to v4.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants