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

Add topp tool selection #69

Open
wants to merge 3 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 the selection of TOPP tools to the github workflows. Copying only required TOPP tools has two advantages:

  1. Reduction of package size
  2. Quicker extraction as on the first install Windows Defender scans all binaries, causing significant extraction times (I observed up to 20 minutes).

This should improve the situation with #65.


PR Type

enhancement, configuration changes


Description

  • Added environment variables OPENMS_VERSION and TOPP_TOOLS to specify the OpenMS version and required TOPP tools.
  • Updated the GitHub Actions workflows to copy only the specified TOPP tools instead of the entire openms-bin directory.
  • This change reduces package size and speeds up extraction times, especially on Windows where antivirus scans can significantly delay the process.

Changes walkthrough 📝

Relevant files
Configuration changes
build-windows-executable-app-with-pyinstaller.yaml
Selective copying of TOPP tools in PyInstaller workflow   

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

  • Added environment variables for OpenMS version and TOPP tools.
  • Modified the script to copy only specified TOPP tools instead of the
    entire openms-bin directory.
  • +9/-4     
    build-windows-executable-app.yaml
    Selective copying of TOPP tools in build workflow               

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

  • Added environment variables for OpenMS version and TOPP tools.
  • Modified the script to copy only specified TOPP tools instead of the
    entire openms-bin directory.
  • +9/-4     

    💡 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] 2
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The PR introduces a change where only specified TOPP tools are copied to the distribution directories. Ensure that all necessary dependencies of these tools are also copied, as missing dependencies could cause runtime failures.

    Copy link

    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
    Add error handling to the file copying process to manage missing files gracefully

    To ensure that the script fails gracefully if the specified .exe files do not exist,
    consider adding error handling around the Copy-Item command. This could involve checking
    if the file exists before attempting to copy it, and logging an error or skipping the copy
    if it does not.

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

    -Copy-Item "openms-bin/${file}.exe" -Destination "dist/bin/${file}.exe"
    +if (Test-Path "openms-bin/${file}.exe") {
    +  Copy-Item "openms-bin/${file}.exe" -Destination "dist/bin/${file}.exe"
    +} else {
    +  Write-Output "Warning: File ${file}.exe does not exist."
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding error handling ensures that the script fails gracefully if the specified .exe files do not exist, which enhances the reliability and robustness of the deployment process.

    9
    Implement error handling for file copying to ensure the workflow does not fail silently

    To enhance the reliability of the deployment script, add error handling for the Copy-Item
    command in the workflow. This ensures that any issues during the file copying are caught
    and handled, preventing the workflow from failing silently if an expected file is missing.

    .github/workflows/build-windows-executable-app.yaml [222]

    -Copy-Item "openms-bin/${file}.exe" -Destination "streamlit_exe/bin/${file}.exe"
    +if (Test-Path "openms-bin/${file}.exe") {
    +  Copy-Item "openms-bin/${file}.exe" -Destination "streamlit_exe/bin/${file}.exe"
    +} else {
    +  Write-Output "Warning: File ${file}.exe does not exist."
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion enhances the reliability of the deployment script by adding error handling for the Copy-Item command, ensuring that any issues during file copying are caught and managed appropriately.

    9
    Possible issue
    Improve the robustness of the script by handling potential spaces in tool names more safely

    Consider using a more robust method to handle potential spaces in the TOPP_TOOLS
    environment variable values. Using -split ' ' assumes that the tool names do not contain
    spaces. If they do, this could lead to incorrect paths or filenames. A safer approach
    might be to use a character that is less likely to be part of a filename, such as a comma,
    and adjust the split accordingly.

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

    -$files = $env:TOPP_TOOLS -split ' '
    +$files = $env:TOPP_TOOLS -split ','
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion addresses a potential issue with spaces in tool names, which could lead to incorrect paths or filenames. Using a comma as a delimiter is a safer approach and improves the robustness of the script.

    8
    Use a comma as a delimiter in TOPP_TOOLS to better handle tool names with spaces

    Similar to the previous suggestion, consider changing the delimiter used in the TOPP_TOOLS
    environment variable to handle potential spaces in tool names. Using a comma as a
    delimiter can help avoid issues if tool names include spaces.

    .github/workflows/build-windows-executable-app.yaml [220]

    -$files = $env:TOPP_TOOLS -split ' '
    +$files = $env:TOPP_TOOLS -split ','
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Similar to the first suggestion, this addresses the potential issue of spaces in tool names, improving the robustness of the script by using a safer delimiter.

    8

    @t0mdavid-m
    Copy link
    Member Author

    Maybe this PR should wait until the next release as well as there seems to be no release yet where SiriusExport.exe is available.

    @Arslan-Siraj
    Copy link
    Contributor

    Thanks @t0mdavid-m

    This was referenced Jul 2, 2024
    @t0mdavid-m t0mdavid-m closed this Sep 25, 2024
    @t0mdavid-m t0mdavid-m deleted the only_copy_required_topp_tools branch September 25, 2024 07:30
    @t0mdavid-m t0mdavid-m restored the only_copy_required_topp_tools 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.

    2 participants