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

Install all dependencies #68

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

t0mdavid-m
Copy link
Member

@t0mdavid-m t0mdavid-m commented Jun 29, 2024

User description

This PR adds portable versions of all dependencies to the windows installations using the cpack routine from the installer.

Note that this required some changes to the OpenMS build configuration and is thus dependent on the following PR in the main repository:

OpenMS/OpenMS#7529

This should fix #67.


PR Type

enhancement, configuration changes


Description

  • Added a new packaging step using ctest and cpack to both GitHub Actions workflows.
  • Replaced separate artifact uploads for bin and share directories with a single package upload.
  • Updated artifact download and extraction steps to handle the new package format.
  • Removed steps for deleting individual bin and share artifacts, replaced with package artifact deletion.

Changes walkthrough 📝

Relevant files
Enhancement
build-windows-executable-app-with-pyinstaller.yaml
Add packaging step and consolidate artifact handling         

.github/workflows/build-windows-executable-app-with-pyinstaller.yaml

  • Added a new packaging step using ctest and cpack.
  • Replaced separate artifact uploads for bin and share directories with
    a single package upload.
  • Updated artifact download and extraction steps to handle the new
    package format.
  • Removed steps for deleting individual bin and share artifacts,
    replaced with package artifact deletion.
  • +28/-26 
    build-windows-executable-app.yaml
    Add packaging step and consolidate artifact handling         

    .github/workflows/build-windows-executable-app.yaml

  • Added a new packaging step using ctest and cpack.
  • Replaced separate artifact uploads for bin and share directories with
    a single package upload.
  • Updated artifact download and extraction steps to handle the new
    package format.
  • Removed steps for deleting individual bin and share artifacts,
    replaced with package artifact deletion.
  • +26/-24 

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 3
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    Ensure that the new packaging and artifact handling steps are correctly configured and tested. The changes involve significant modifications to the CI/CD pipeline, which could potentially break the build or deployment process if not properly implemented.
    Dependency Check:
    Verify that the new dependencies and the changes made in the linked PR (https://github.com/OpenMS/OpenMS/pull/7529) are compatible and do not introduce any conflicts or issues in the build process.

    Copy link

    codiumai-pr-agent-pro bot commented Jun 29, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Improve the specificity of the zip extraction command to ensure only relevant artifacts are processed

    Consider using a more specific pattern for the zip files to be extracted to avoid
    unintended file extractions and ensure only relevant artifacts are processed. This can be
    crucial in a CI environment where multiple jobs might produce different artifacts.

    .github/workflows/build-windows-executable-app-with-pyinstaller.yaml [180]

    -unzip "*.zip" -d .
    +unzip "openms-*.zip" -d .
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion improves the robustness of the CI process by ensuring that only relevant artifacts are extracted, which is crucial in environments with multiple jobs producing different artifacts.

    9
    Best practice
    Add error handling to the unzip operation to enhance the robustness of the workflow

    It's a good practice to check the success of critical operations such as unzipping
    artifacts. Adding error handling for the unzip operation can prevent subsequent steps from
    running if the unzip fails, which can save time and resources in the CI process.

    .github/workflows/build-windows-executable-app-with-pyinstaller.yaml [180]

    -unzip "*.zip" -d .
    +unzip "*.zip" -d . || exit 1
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling to the unzip operation is a good practice that can prevent subsequent steps from running if the unzip fails, saving time and resources in the CI process.

    8
    Standardize the version of actions/download-artifact used across the workflow to ensure consistent behavior

    To maintain consistency and avoid potential issues with different versions of actions,
    consider using the same version for all instances of actions/download-artifact. This
    change ensures that all artifact downloads are handled consistently.

    .github/workflows/build-windows-executable-app-with-pyinstaller.yaml [172]

    -uses: actions/download-artifact@v4
    +uses: actions/download-artifact@v3
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a consistent version of actions/download-artifact can help avoid potential issues caused by version discrepancies, although the current version (v4) is more recent and likely has improvements over v3.

    7
    Maintainability
    ✅ Define environment variables at the job level to ensure consistent access across all steps
    Suggestion Impact:The suggestion to define environment variables at the job level was implemented. The environment variables OPENMS_VERSION and TOPP_TOOLS were moved to the job level.

    code diff:

    +env:
    +  OPENMS_VERSION: 3.0.0 
    +  # Define needed TOPP tools here
    +  TOPP_TOOLS: "FeatureFinderMetabo MetaboliteAdductDecharger SiriusExport"

    To avoid potential conflicts and ensure that the environment variables are correctly
    scoped, consider defining SOURCE_DIRECTORY and other related environment variables at the
    job level instead of within individual steps. This change ensures that all steps within
    the job have consistent access to these variables.

    .github/workflows/build-windows-executable-app-with-pyinstaller.yaml [142-147]

    -SOURCE_DIRECTORY: "${{ github.workspace }}/OpenMS"
    +env:
    +  SOURCE_DIRECTORY: "${{ github.workspace }}/OpenMS"
    +  PACKAGE_TYPE: nsis
    +  SEARCH_ENGINES_DIRECTORY: "${{ github.workspace }}/_thirdparty"
    +  CI_PROVIDER: "GitHub-Actions"
    +  PACK_ZIP: 1
    +  CPACK_PACKAGE_FILE_NAME: "openms-package"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Defining environment variables at the job level can improve maintainability and ensure consistent access across all steps, but it is not critical as the current implementation is functional.

    6

    @t0mdavid-m
    Copy link
    Member Author

    t0mdavid-m commented Jun 29, 2024

    The workflow is currently failing since cipackage.cmake was introduced after the 3.0.0 release. However, until the PR mentioned above is merged and a release is created, there wont be an available release that supports the new workflow. It is working fine for FLASHViewerwhich uses a custom branch of OpenMS with cherry picked commits from the PR.

    @t0mdavid-m t0mdavid-m mentioned this pull request Jul 2, 2024
    @t0mdavid-m
    Copy link
    Member Author

    I updated the delete artifact version (see #71)

    @t0mdavid-m
    Copy link
    Member Author

    The PR (OpenMS/OpenMS#7529) in the main repository is now merged. We decided on different flags which I just updated for this PR as well.

    @t0mdavid-m t0mdavid-m closed this Sep 25, 2024
    @t0mdavid-m t0mdavid-m deleted the install_all_dependencies branch September 25, 2024 07:29
    @t0mdavid-m t0mdavid-m restored the install_all_dependencies branch September 25, 2024 08:05
    @t0mdavid-m t0mdavid-m reopened this Sep 25, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    TOPP Tools in Windows executables
    1 participant