geant4: new version 10.6 plus simplifications by drbenmorgan · Pull Request #15447 · spack/spack

drbenmorgan · 2020-03-11T17:19:39Z

This is a fairly major refactor and simplification of Geant4 and its associated data packages, and will changes hashes of previous installs. I think this is worthwhile for long term maintenance of the package, so this PR is "Draft" for now to get early eyes on it and discuss with interested parties.

The primary change is the removal of the data variant so the physics model data packages are always installed by the g4XXX packages, then used by Geant4 itself through the geant4-data package. geant4-data is now a BundlePackage whose version maps to that of geant4 and depends on the g4XXX packages at the exact versions the geant4 version supports. It creates a "view" (more on that later) of the data so Geant4's install mechanisms sees a single directory. Each g4XXX package also now sets the appropriate runtime environment.

I believe removal of the data variant is worthwhile as the external data is platform/arch independent, so really benefits reuse and reducing install sizes.

The second breaking change is to move the versioning to "standard" semantic versioning, so 10.6.0 rather than 10.06.p00. This is just to make the packaging cleaner and simple, plus the use of the Geant4 GitLab repo.

The remainder of the updates are pure simplification and cleanup.

One potential issue to look at is the "view" creation in geant4-data. All this does is create a directory <prefix>/share/geant4-data-<version>/ and create symlinks in that to the g4xxx data directories. That works, and Geant4's seems fine with it. However, if a spack environment/view is created and geant4-data installed in this, the symlinks under <viewprefix>/share/geant4-data-<version>/ are created as normal but empty directories.

I'm not sure this is critical as the environment variables needed by Geant4 are set correctly, but feels like I've missed something.

drbenmorgan · 2020-03-11T17:21:46Z

It would also be best to wait for @sethrj 's #14520 on vecgeom to be merged so we can take advantage of it for that dependence.

ChristianTackeGSI

Going to look at the geant4/geant4-data part tomorrow.

ChristianTackeGSI · 2020-03-12T17:05:44Z

var/spack/repos/builtin/packages/g4abla/package.py

+    def setup_dependent_run_environment(self, env, dependent_spec):
+        install_path = join_path(prefix.share, 'data', 'G4ABLA{0}'


Does this actually work?
Where is just prefix set?

Could you try this?

Suggested change

def setup_dependent_run_environment(self, env, dependent_spec):

install_path = join_path(prefix.share, 'data', 'G4ABLA{0}'

def setup_dependent_run_environment(self, env, dependent_spec):

install_path = join_path(self.prefix.share.data, 'G4ABLA{0}'

Fixed in 0d4a05a

ChristianTackeGSI · 2020-03-12T17:07:25Z

var/spack/repos/builtin/packages/g4incl/package.py

+    def url_for_version(self, version):
+        """Handle version string."""
+        return ("http://geant4-data.web.cern.ch/geant4-data/datasets/G4INCL.%s.tar.gz" % version)


Is this really needed? Will the normal version interpolation into url really fail?

Unfortunately yes, due to the use of . between the package name and version, combined with the name including a number. Spack interprets the version as 4INCL.X.Y.

Add new 10.6.0 release, migrating download of source to use Geant4's public release repo on CERN GitLab. Change versioning scheme to use clearer and standard semantic scheme. Update geant4-data and g4XXX data packages with new versions. Migrate geant4-data as a BundlePackage of the g4XXX packages, installing links to each under a single directory under share for geant4-data. Ensure each g4XXX package exports the environment variable pointing to its location expected by Geant4. Remove "data" variant from Geant4 package and always use geant4-data. Simplify cxxstd variant transport to dependencies.

Geant4 major.minor versions have specific dependencies on vecgeom versions. Add missing vecgeom version for geant4 10.5, and match version requirements for vecgeom in geant4 depends_on.

drbenmorgan · 2020-03-23T15:06:04Z

Now rebased on develop and added an additional vecgeom version needed by Geant4 10.5. I just need to test locally and tidy up the options, then this'll be ready for final review/test.

drbenmorgan · 2020-03-24T12:35:15Z

O.k., I've done basic install tests and defaults seem to work fine. I think this is ready for final review checks and tests.

ChristianTackeGSI

Looks good.
I didn't test it yet.

ChristianTackeGSI · 2020-03-25T12:05:09Z

var/spack/repos/builtin/packages/geant4-data/package.py

-        url = url + 'ReleaseNotes4.{0}.{1}.html'.format(version[0], version[1])
-        return url
+            for s in spec.dependencies():
+                for d in glob.glob('{0}/share/data/*'.format(s.prefix)):


Maybe use s.prefix.share to be more consistent with the rest of the patch?

ChristianTackeGSI · 2020-03-25T12:30:44Z

var/spack/repos/builtin/packages/geant4/package.py

+
+        # install the data with geant4
+        datadir = spec['geant4-data'].prefix.share
+        dataver = 'geant4-data-{0}'.format(spec['geant4-data'].version)


Maybe to be consistent with the setup in geant4-data?

Suggested change

dataver = 'geant4-data-{0}'.format(spec['geant4-data'].version)

dataver = '{0}-{1}'.format(spec['geant4-data']..name, spec['geant4-data'].version.dotted)

Also I wonder, if geant4-data could have a property/value that could be used here?

ChristianTackeGSI · 2020-03-25T12:45:00Z

Looking at python's def command(self) maybe this can be done on the geant-data class:

    @property
    def geant4_datadir(self):
        verdir = '{0}-{1}'.format(self.name, self.version.dotted)
        return join_path(self.prefix.share, 'data', verdir)

UNTESTED!

var/spack/repos/builtin/packages/geant4/package.py

drbenmorgan · 2020-03-30T15:07:16Z

Thanks @ChristianTackeGSI and @sethrj for the review! I think I've addressed these in the latest commits, so barring flake8 errors and any further comments, I think this is good for merge.

drbenmorgan · 2020-03-31T08:53:50Z

Whilst the coverage checks are failing, the changes in this PR aren't (at least obviously) touching Spack's core, which the coverage handles (thanks to @alalazo for the info). I'm going to merge in this case (and I'm maintainer of these packages, so I'll get pinged if there are issues!).

* origin/develop: packages.yaml: allow virtuals to specify buildable: false (#14934) GMT: add 6.0.0 (#15785) autoconf-archive: add new package (#15782) JasPer: add 2.0.16 (#15781) Update link for codecov's browser extensions (#15779) geant4: new version 10.6 plus simplifications (#15447) ppOpen-APPL/FVM: new package. (#15772)

drbenmorgan requested review from gartung and greenc-FNAL March 11, 2020 17:19

ChristianTackeGSI reviewed Mar 12, 2020

View reviewed changes

drbenmorgan added don't-merge-yet hep labels Mar 13, 2020

gartung approved these changes Mar 13, 2020

View reviewed changes

drbenmorgan mentioned this pull request Mar 17, 2020

Update and exetend VecGeom #14520

Merged

drbenmorgan added 5 commits March 23, 2020 13:28

g4<DATA>: Use self to resolve correct prefix

e2fb003

geant4, data: Fix flake8 errors

f716c06

g4photonevaporation: flake8 fix

968d33d

geant4: vecgeom version depends_on

3d8807b

Geant4 major.minor versions have specific dependencies on vecgeom versions. Add missing vecgeom version for geant4 10.5, and match version requirements for vecgeom in geant4 depends_on.

drbenmorgan force-pushed the update-geant4 branch from 7149b92 to 3d8807b Compare March 23, 2020 15:03

drbenmorgan marked this pull request as ready for review March 24, 2020 12:32

drbenmorgan removed the don't-merge-yet label Mar 24, 2020

ChristianTackeGSI approved these changes Mar 25, 2020

View reviewed changes

sethrj reviewed Mar 27, 2020

View reviewed changes

var/spack/repos/builtin/packages/geant4/package.py Outdated Show resolved Hide resolved

drbenmorgan added 3 commits March 30, 2020 14:49

geant4: c++17 patch specific for 10.4.3

5610c3f

geant4: simplify geant4-data setup

84363d6

geant4: Use new define_from_variant function

6cda86c

gartung approved these changes Mar 30, 2020

View reviewed changes

geant4: fix flake8 errors

79425ca

drbenmorgan merged commit 77e1384 into spack:develop Mar 31, 2020

		def setup_dependent_run_environment(self, env, dependent_spec):
		install_path = join_path(prefix.share, 'data', 'G4ABLA{0}'

	dataver = 'geant4-data-{0}'.format(spec['geant4-data'].version)
	dataver = '{0}-{1}'.format(spec['geant4-data']..name, spec['geant4-data'].version.dotted)

Conversation

drbenmorgan commented Mar 11, 2020

Uh oh!

drbenmorgan commented Mar 11, 2020

Uh oh!

ChristianTackeGSI left a comment

Choose a reason for hiding this comment

Uh oh!

ChristianTackeGSI Mar 12, 2020

Choose a reason for hiding this comment

Uh oh!

drbenmorgan Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

ChristianTackeGSI Mar 12, 2020

Choose a reason for hiding this comment

Uh oh!

drbenmorgan Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

drbenmorgan commented Mar 23, 2020

Uh oh!

drbenmorgan commented Mar 24, 2020

Uh oh!

ChristianTackeGSI left a comment

Choose a reason for hiding this comment

Uh oh!

ChristianTackeGSI Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

ChristianTackeGSI Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

ChristianTackeGSI commented Mar 25, 2020

Uh oh!

Uh oh!

drbenmorgan commented Mar 30, 2020

Uh oh!

drbenmorgan commented Mar 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants