Skip to content

GHA: use Ubuntu 22.04 - #943

Merged
KrisThielemans merged 9 commits into
SyneRBI:masterfrom
paskino:ubuntu-22-build
Mar 21, 2025
Merged

GHA: use Ubuntu 22.04#943
KrisThielemans merged 9 commits into
SyneRBI:masterfrom
paskino:ubuntu-22-build

Conversation

@paskino

@paskino paskino commented Mar 11, 2025

Copy link
Copy Markdown
Contributor

Updates the GHA to explicitly use Ubuntu 22.04.

Adds the zenodo_get dependency in requirements.txt.

@paskino
paskino requested a review from KrisThielemans March 11, 2025 20:23

@KrisThielemans KrisThielemans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess you want to split #937 into 2. However, you currently seem to default to not using the Ubuntu system libraries, but

Also, come to think of it, why "UBUNTU" in USE_UBUNTU_SYSTEM_LIBRARIES? (It'll work on any debian system).

In any case, do you really want the USE_UBUNTU_SYSTEM_LIBRARIES stuff in this PR?

Comment thread docker/build_gadgetron-ubuntu.sh Outdated
golang
pkg-config

if [ ${USE_UBUNTU_SYSTEM_LIBRARIES} ] ; then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this will fail if it isn't set, and in any case, test 1 is probably a very strange thing. I think you need something like

# set default value
: ${USE_UBUNTU_SYSTEM_LIBRARIES:=0}
if [ ${USE_UBUNTU_SYSTEM_LIBRARIES} == 1] ; then
...

@paskino

paskino commented Mar 12, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for your comments @KrisThielemans . Indeed my aim was to split #937 in subtasks, but I forgot to account for the VM, for instance.

I was aiming at fixing docker with a second PR, keeping these separated.

As we use the same build scripts for docker, VM and straight Ubuntu systems (the VM is one of them), we need to be able to differentiate what to install and what not.

I understand now that I should not have broken backward compatibility.

Probably, I should have added a variable that is INSTALL_PREREQUISITES_VIA_CONDA. Does it make sense?

@KrisThielemans

Copy link
Copy Markdown
Member

Probably, I should have added a variable that is INSTALL_PREREQUISITES_VIA_CONDA. Does it make sense?

that's a good idea. We could default it to 0 now, and switch to 1 later. But maybe you want to do that in the 2nd PR anyway (in which case this PR would be really small). As you prefer.

Comment thread .github/workflows/c-cpp.yml Outdated
not appropriate for this PR
Comment thread .github/workflows/c-cpp.yml Outdated

@KrisThielemans KrisThielemans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please squash-merge (with edited commit message) when GHA is happy

@KrisThielemans
KrisThielemans merged commit 82466ed into SyneRBI:master Mar 21, 2025
@KrisThielemans KrisThielemans mentioned this pull request Jun 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants