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.

Sign up for GitHub

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

Jump to bottom

update CIL and CCPi-Regularisation version and reinstate tests #897

Merged
merged 25 commits into from
Jun 10, 2024
Merged

update CIL and CCPi-Regularisation version and reinstate tests #897

merged 25 commits into from
Jun 10, 2024

Conversation

paskino
Copy link
Contributor

@paskino paskino commented May 13, 2024 •
edited
Loading

Bump CIL and CCPi-Regularisation-Toolkit versions for PSMR24 training school

Currently failing build with BUILD_ROOT=ON . I don't undestand where this comes from as I did not change anything around this.

CMake Error at /home/runner/work/SIRF-SuperBuild/SIRF-SuperBuild/build/builds/ROOT/build/builtins/xrootd/XROOTD-prefix/src/XROOTD-stamp/XROOTD-install-Release.cmake:49 (message):
  Command failed: 1

   '/usr/local/bin/cmake' '-Dmake=' '-Dconfig=' '-P' '/home/runner/work/SIRF-SuperBuild/SIRF-SuperBuild/build/builds/ROOT/build/builtins/xrootd/XROOTD-prefix/src/XROOTD-stamp/XROOTD-install-Release-impl.cmake'

@paskino paskino mentioned this pull request May 13, 2024
Merged
6 tasks
KrisThielemans
KrisThielemans reviewed May 13, 2024
View reviewed changes
Copy link
Member

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

Is there anything here for me to check?

Obviously, merge the SIRF PR first and set SIRF_TAG accordingly

@paskino
Copy link
Contributor Author

paskino commented May 14, 2024

@KrisThielemans do you have any clue onto why ROOT fails to build with these changes?

@KrisThielemans
Copy link
Member

KrisThielemans commented May 14, 2024 •
edited by paskino
Loading

https://github.com/SyneRBI/SIRF-SuperBuild/actions/runs/9074911667/job/24934505517?pr=897#step:10:10217

[  0%] Performing install step for 'XROOTD'
CMake Error at /home/runner/work/SIRF-SuperBuild/SIRF-SuperBuild/build/builds/ROOT/build/builtins/xrootd/XROOTD-prefix/src/XROOTD-stamp/XROOTD-install-Release.cmake:49 (message):
  Command failed: 1
   '/usr/local/bin/cmake' '-Dmake=' '-Dconfig=' '-P' '/home/runner/work/SIRF-SuperBuild/SIRF-SuperBuild/build/builds/ROOT/build/builtins/xrootd/XROOTD-prefix/src/XROOTD-stamp/XROOTD-install-Release-impl.cmake'
  See also
    /home/runner/work/SIRF-SuperBuild/SIRF-SuperBuild/build/builds/ROOT/build/builtins/xrootd/XROOTD-prefix/src/XROOTD-stamp/XROOTD-install-*.log

Luckily, we do upload logs as an artefact (visible in the Summary of the Action). Checking the install-err.log:

gmake[7]: *** read jobs pipe: Bad file descriptor.  Stop.
gmake[7]: *** Waiting for unfinished jobs....
gmake[6]: *** [Makefile:136: all] Error 2

We have had this before root-project/root#14520 and it auto-disappeared.

Presumably our build is overload the MS network... Sadly, I'm not sure what can be done about it. I suppose we could modify the job to use a packaged ROOT as opposed to building it ourselves, and have one that does build it, but is allowed to fail.

@KrisThielemans
Copy link
Member

Here's what we do in STIR GHA
https://github.com/UCL/STIR/blob/feb6d85eadb392f5b8278d3b97ae2ee67ca439d9/.github/workflows/build-test.yml#L260-L264
You'd then have to say USE_SYSTEM_ROOT=ON. Of course, it would make sense to put the download in the docker scripts. Not 100% sure about the source .../thisroot.sh though.

@paskino
Copy link
Contributor Author

paskino commented May 14, 2024

Does this mean we will not test BUILD_ROOT=ON rather USE_SYSTEM_ROOT=ON on the CI?

@KrisThielemans
Copy link
Member

yes, I think it'd be nice to have one job where we build it ourselves, but allow failure, as I wrote.

@paskino paskino mentioned this pull request May 14, 2024
Closed
casperdcl
casperdcl requested changes May 15, 2024
View reviewed changes
Dockerfile Show resolved Hide resolved
docker/user_demos.sh Outdated Show resolved Hide resolved
casperdcl
casperdcl reviewed May 15, 2024
View reviewed changes
docker/user_demos.sh Outdated Show resolved Hide resolved
version_config.cmake Outdated Show resolved Hide resolved
casperdcl
casperdcl reviewed May 15, 2024
View reviewed changes
docker/user_demos.sh Outdated Show resolved Hide resolved
casperdcl
casperdcl requested changes May 16, 2024
View reviewed changes
Copy link
Member

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

fix bugs

Dockerfile Outdated Show resolved Hide resolved
docker/user_demos.sh Outdated Show resolved Hide resolved
@KrisThielemans
Copy link
Member

Please merge master to get the correct STIR version.

@KrisThielemans KrisThielemans mentioned this pull request May 30, 2024
Closed
47 tasks
@KrisThielemans
Copy link
Member

@paskino @casperdcl this is ready to merge from my perspective. Please ahead, and then tag.

@casperdcl
Copy link
Member

casperdcl commented May 31, 2024 •
edited
Loading

IIRC @paskino found another bug in this PR

casperdcl
casperdcl reviewed May 31, 2024
View reviewed changes
Dockerfile Show resolved Hide resolved
KrisThielemans
KrisThielemans reviewed May 31, 2024
View reviewed changes
env_sirf.sh.in Outdated Show resolved Hide resolved
paskino
paskino commented Jun 6, 2024
View reviewed changes
version_config.cmake Outdated Show resolved Hide resolved
@paskino
Copy link
Contributor Author

paskino commented Jun 6, 2024

waiting for

casperdcl
casperdcl requested changes Jun 6, 2024
View reviewed changes
Copy link
Member

@casperdcl casperdcl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I merged TomographicImaging/CCPi-Regularisation-Toolkit#205 and released it as v24.0.1... I presume you manually jenkins will upload it to conda -c ccpi

CHANGES.md Outdated Show resolved Hide resolved
version_config.cmake Outdated Show resolved Hide resolved
@paskino @casperdcl
update to newest CCPi code versions
7a0ac56 
Co-authored-by: Casper da Costa-Luis <casper.dcl@physics.org>
@paskino paskino mentioned this pull request Jun 6, 2024
Closed
paskino and others added 2 commits June 6, 2024 12:40
@paskino
add cil requirements as we delete cil if BUILD_CIL=ON
1cb17ad
@paskino @casperdcl
update changelog
a73251d 
Co-authored-by: Casper da Costa-Luis <casper.dcl@physics.org>
@paskino paskino requested a review from casperdcl June 7, 2024 09:26
KrisThielemans
KrisThielemans requested changes Jun 7, 2024
View reviewed changes
Copy link
Member

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

tiny change. then merge!

Dockerfile Outdated Show resolved Hide resolved
@KrisThielemans
Copy link
Member

@casperdcl @paskino please merge and tag

casperdcl
casperdcl requested changes Jun 10, 2024
View reviewed changes
docker/user_demos.sh Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@casperdcl

This comment was marked as off-topic.

Sign in to view
@casperdcl casperdcl merged commit ec7905b into master Jun 10, 2024
8 checks passed
@casperdcl casperdcl deleted the bump_cil_modules branch June 10, 2024 12:12
@casperdcl casperdcl mentioned this pull request Jun 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@casperdcl casperdcl casperdcl requested changes

@KrisThielemans KrisThielemans KrisThielemans requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CCPi regularisation fails test
3 participants
@paskino @KrisThielemans @casperdcl