sentieon-genomics(GENESIS): add 2.0.x, 2.1.x, tests, fix for A64FX #1891
sentieon-genomics(GENESIS): add 2.0.x, 2.1.x, tests, fix for A64FX #1891chig wants to merge 8 commits into
Conversation
|
This pull request has been automatically marked as stale because it has not had any activity in the last 6 months. It will be closed in 30 days if there is no further activity. |
|
@spack/spack-releasers |
| options.extend(self.enable_or_disable("hmdisk")) | ||
| if spec.satisfies("+cuda"): | ||
| options.append("--enable-gpu") | ||
| options.append("--with-cuda=%s" % spec["cuda"].prefix) |
There was a problem hiding this comment.
| options.append("--with-cuda=%s" % self.spec["cuda"].prefix) |
|
Thank you very much for your suggestion. I've updated the recipe. |
|
Thanks for the review. I believe I have addressed the requested changes:
I’ve requested review again. Thank you for taking another look. |
|
@alalazo I believe I have addressed the remaining requested change by removing the obsolete Also, the workflow is currently awaiting maintainer approval, so I would appreciate it if the CI could be approved to run. |
alalazo
left a comment
There was a problem hiding this comment.
Just one small comment. I'll let CI run in the meanwhile
|
I've updated package.py using style fix. |
bernhardkaindl
left a comment
There was a problem hiding this comment.
@alalazo, I guess your GitHub-requested changes were applied,
PR would be ready to merge?
Checked: sentieon-genomics@202503.03 installs on Linux
bernhardkaindl
left a comment
There was a problem hiding this comment.
alalazo / @chig, these are comments from GitHub Copilot from review of the PR on 25.6.2006:
Latest Updated Review Summary (25.6.2006)
GitHub Copilot checked the earlier changes requested by @alalazo, and they appear to be applied:
- old versions/resources were removed
- obsolete patch directives/files were removed
- the obsolete
@mastercondition was removed - the
--host=Fugakulogic was narrowed to thea64fxtarget - explanatory comments were added above the patch directives
It also did another pass over the current diff and found one remaining risk worth addressing before merge:
force_autoreconf = Trueis now unconditional, andautoconf,automake,libtool, andm4are unconditional build dependencies for all builds.
Previously, autoreconf and those autotools dependencies were only needed for the patched Fujitsu-specific cases. Making them unconditional broadens the build behavior for every version and compiler, not just %fj. That means this PR is no longer only adding versions / updating URLs / %fj patch handling; it also changes the default build path for non-Fujitsu builds.
Copilot thinks this should either:
- be limited back to the cases that actually patch autotools input files, or
- be justified explicitly if all supported versions now require autoreconf.
Other than that, the earlier requested changes from @alalazo look applied to me.
Detailed review:
Q: Can you check if the changes requested by @alalazo by reviewing with changes-requested status have been applied? Also, can you review the PR for further issues or risks and add a comment to the PR with your results?
The specific changes requested by @alalazo in the earlier changes-requested reviews appear to have been applied, but GitHub Copilot also found one substantive remaining risk in the current PR diff.
What I checked:
@alalazo requested removal of old versions/resources: applied.
@alalazo requested removal of obsolete patch directives/files: applied.
@alalazo requested fixing the obsolete @master condition: applied.
@alalazo requested keeping the --host=Fugaku logic only for a64fx: applied by changing the condition to spec.target == "a64fx".
@alalazo later requested comments above patch directives explaining effect/need/source: partially applied. The PR now has explanatory comments for the patch directives, but I did not verify provenance/source text for each patch beyond what is visible in the current diff.
Further review result:
I found a likely regression risk in package.py: the PR sets force_autoreconf = True unconditionally and makes autoconf, automake, libtool, and m4 unconditional build dependencies for all versions. Previously those were limited to the Fujitsu-patched builds / @master path. Since the main purpose of the PR is adding packaged releases and patching Fujitsu builds, broadening autoreconf to every build changes behavior for non-Fujitsu builds too. That may be intentional, but it is a materially wider change than the review thread established and could introduce avoidable build risk.
Why this matters:
Running autoreconf on every build can change the generated configure machinery for versions that previously built from shipped release tarballs without regeneration.
Making autotools dependencies unconditional increases the dependency surface for all users, not only the %fj cases that seem to require patching configure.ac.
|
I rebased the branch on the current develop branch and added a comment explaining why autoreconf is required unconditionally for GENESIS GitHub source archives. |
The initial description was empty, updated by @bernhardkaindl:
This PR
alalazo / @chig, these are comments from GitHub Copilot from review of the PR on 25.6.2006:
Latest Updated Review Summary (25.6.2006)
GitHub Copilot checked the earlier changes requested by @alalazo, and they appear to be applied:
@mastercondition was removed--host=Fugakulogic was narrowed to thea64fxtargetIt also did another pass over the current diff and found one remaining risk worth addressing before merge:
force_autoreconf = Trueis now unconditional, andautoconf,automake,libtool, andm4are unconditional build dependencies for all builds.Previously, autoreconf and those autotools dependencies were only needed for the patched Fujitsu-specific cases. Making them unconditional broadens the build behavior for every version and compiler, not just
%fj. That means this PR is no longer only adding versions / updating URLs /%fjpatch handling; it also changes the default build path for non-Fujitsu builds.Copilot thinks this should either:
Other than that, the earlier requested changes from @alalazo look applied to me.
Detailed review:
Q: Can you check if the changes requested by @alalazo by reviewing with changes-requested status have been applied? Also, can you review the PR for further issues or risks and add a comment to the PR with your results?
The specific changes requested by @alalazo in the earlier changes-requested reviews appear to have been applied, but GitHub Copilot also found one substantive remaining risk in the current PR diff.
Checked:
Further review result:
Why this matters: