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

Update dependencies #2258

Closed
wants to merge 11 commits into from
Closed

Update dependencies #2258

wants to merge 11 commits into from

Conversation

jburel
Copy link
Member

@jburel jburel commented Jun 30, 2022

  • Update dependencies as part of the daily run of GHA
  • Update model object

The proposed changes can be tested by running bash omero/dependencies.sh (jq and omego need to be installed)
This will avoid the manual upgrade of the dependencies
I have renamed 3 variables to make the parsing easier and avoid unnecessary if statement

@jburel jburel requested a review from sbesson June 30, 2022 06:33
@snoopycrimecop
Copy link
Member

snoopycrimecop commented Jul 1, 2022

Conflicting PR. Removed from build OMERO-docs#1273. See the console output for more details.
Possible conflicts:

  • Upstream changes
    • omero/autogen_db_version.py

--conflicts Conflict resolved in build OMERO-docs#1274. See the console output for more details.

@jburel
Copy link
Member Author

jburel commented Jul 1, 2022

I have been thinking about it a bit more, I need to find a better way to handle the links of openmicroscopy dependencies stack. the chosen approach is making some assumptions that might lead to error

@jburel jburel changed the title Update dependencies WIP: Update dependencies Jul 1, 2022
@jburel jburel mentioned this pull request Jul 2, 2022
@jburel jburel changed the title WIP: Update dependencies Update dependencies Jul 4, 2022
Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

As part of the review, I found omero/autogen_docs and the sub-scripts extremely hard to read and understand. The top-level script already has a lot of logic to pull and combine data from different source and this PR is adding extra steps on top of that. Given this experience, this raises the question of its maintainability as the reviewing burden is substantial (having not even executed the script yet).

One outstanding issue is about the version of the server which should be downloaded/built consistently across the script.

Trying to think how to address the larger question, I wondered whether splitting the uber-script into separate update scripts/workflows per source e.g. omero-install, omeroweb-install, openmicroscopy would help the review and the execution.

@@ -31,7 +31,7 @@ jobs:
- name: Update documentation
run: |
python -m venv $space/.venv3
$space/.venv3/bin/pip install -U jinja2==3.0.1
$space/.venv3/bin/pip install -U jinja2==3.0.1 omego
Copy link
Member

@sbesson sbesson Jul 6, 2022

Choose a reason for hiding this comment

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

Why this requirement? (edit: spotted a typo in the dependencies.sh script)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was using omego to download but this will be removed when using head

dirs=("OMERO.server/lib/server/omero-blitz.jar" "OMERO.server/lib/server/omero-server.jar" "OMERO.server/lib/server/omero-gateway.jar"
"OMERO.server/lib/server/omero-romio.jar" "OMERO.server/lib/server/omero-renderer.jar" "OMERO.server/lib/server/omero-common.jar"
"OMERO.server/lib/server/omero-model.jar" "OMERO.server/lib/server/formats-gpl.jar")
omero download --release 5
Copy link
Member

Choose a reason for hiding this comment

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

omego ?

version=`unzip -p $dir META-INF/MANIFEST.MF | grep "Implementation-Version:" | sed 's/^.*[^0-9]\([0-9]*\.[0-9]*\.[0-9]*\).*$/\1/'`
echo $v
echo $version
sed -i -e "s/version_${v} = .*/version_${v} = \"${version}\"/" omero/onf_autogen.py
Copy link
Member

@sbesson sbesson Jul 6, 2022

Choose a reason for hiding this comment

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

I assume it's omero/conf_autogen.py

Copy link
Member Author

Choose a reason for hiding this comment

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

Typo

@@ -14,6 +14,11 @@ WORKSPACE=${WORKSPACE:-$(pwd)}
WORKSPACE=${WORKSPACE%/} # Remove trailing slashes
USER=${USER:-$(whoami)}

echo "Retrieve model object"
URL="https://latest-ci.openmicroscopy.org/jenkins/job/OMERO-server/lastSuccessfulBuild/artifact/EveryObject.rst"
Copy link
Member

@sbesson sbesson Jul 6, 2022

Choose a reason for hiding this comment

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

I am lost on which OMERO.server version sources are being used:

  • the GitHub action checks out and builds the HEAD of ome/openmicroscopy
  • this step downloads an artifact generated by latest-ci (i.e. assuming HEAD)
  • omero dependencies downloads a released server and updates the versions

Shouldn't all of this use the same version?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could use head. I was testing independently and could now reconcile the 2

@@ -86,6 +91,10 @@ done
echo "Getting db properties"
omero/autogen_db_version.py $WORKSPACE/OMERO.server > omero/conf_autogen.py

echo "Update dependencies"
bash omero/dependencies.sh
Copy link
Member

@sbesson sbesson Jul 6, 2022

Choose a reason for hiding this comment

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

The purpose of this script seems to be largely identical to omero/autogen_db_version.py above except it's in Bash vs Python and the new script downloads the latest release server vs using OMERO.server.
What was the rationale for not amending the existing logic?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is all a bit messy since we have bash script invoking python script. I opted for a bash script since the source scrip is a bash one

value=${values[${#values[@]}-1]}
v=${value#"$PREFIX"}
v=${v//"-"/"_"}
echo $v
Copy link
Member

Choose a reason for hiding this comment

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

Are these debugging statements?

Copy link
Member Author

Choose a reason for hiding this comment

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

Helpful to have to follow the process

@jburel
Copy link
Member Author

jburel commented Jul 6, 2022

I will split the original script (I am not the author) and make it more readable and easier to review

@jburel jburel mentioned this pull request Jul 7, 2022
This pull request was closed.
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.

None yet

3 participants