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 GW to v1.1.1 #51426

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Update GW to v1.1.1 #51426

wants to merge 10 commits into from

Conversation

kcleal
Copy link
Contributor

@kcleal kcleal commented Oct 16, 2024

No description provided.

@kcleal
Copy link
Contributor Author

kcleal commented Oct 16, 2024

@BiocondaBot please fetch artifacts

@BiocondaBot
Copy link
Collaborator

Package(s) built are ready for inspection:

Arch Package Zip File / Repodata CI Instructions
linux-64 gw-1.1.1-h68f53fc_0.tar.bz2 LinuxArtifacts.zip Azure
showYou may also use conda to install after downloading and extracting the zip file. From the LinuxArtifacts directory: conda install -c ./packages <package name>
osx-arm64 gw-1.1.1-hc3eed46_0.tar.bz2 repodata.json CircleCI
showYou may also use conda to install:conda install -c https://output.circle-artifacts.com/output/job/83e83254-37e0-4561-8d72-061ad804d0cc/artifacts/0/tmp/artifacts/packages <package name>

Docker image(s) built:

Package Tag CI Install with docker
gw 1.1.1--h68f53fc_0 Azure
showImages for Azure are in the LinuxArtifacts zip file above.gzip -dc LinuxArtifacts/images/gw:1.1.1--h68f53fc_0.tar.gz | docker load

Copy link
Contributor

coderabbitai bot commented Oct 16, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request introduces updates to the meta.yaml file for the gw package. Specifically, the version number has been incremented from "1.1.0" to "1.1.1", and the SHA256 checksum has been modified from "dc96521b8252bde02d9cc68bb3cee156c0469889d2783ee72682338d10657892" to "5ac0430d9179d1a48938f24011c329fe807e72aba0c20eb191c566d129115f86". Additionally, several new test commands have been added under the test section: which gw && ls -lh $(which gw), uname -a, and a conditional command that checks if the system is Darwin to execute file $(which gw). The previous command that generated a test image has been commented out. All other content within the file, including package metadata, source URL, build requirements, and configurations, remains unchanged.

The build.sh script has also been modified, primarily focusing on the build process for a project involving Skia and GLFW. The assignment of the USE_GL variable has been commented out, and the output redirection for make prep has been adjusted. The script now modifies the Makefile and src/plot_manager.cpp files conditionally based on the operating system, with specific changes to linker flags and context API definitions. The script sets various flags based on the OS type and compiles the project, ensuring the correct permissions for the resulting files.

Possibly related PRs

Suggested labels

autobump, new version


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 5adab0a and 43a65a0.

📒 Files selected for processing (1)
  • recipes/gw/build.sh (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
recipes/gw/build.sh (4)

Line range hint 26-36: Approve build and installation process with suggestion to verify CXXFLAGS addition.

The build and installation process looks good:

  1. The make command correctly uses the OS-specific flags set earlier.
  2. File copying and permission setting are appropriate for package installation.

The addition of -D_LIBCPP_DISABLE_AVAILABILITY to CXXFLAGS is notable:

To ensure the necessity and impact of the new CXXFLAGS addition, please run the following script:

#!/bin/bash
# Description: Verify the impact of -D_LIBCPP_DISABLE_AVAILABILITY flag

# Test 1: Check if the flag is necessary for successful compilation
echo "Compiling without -D_LIBCPP_DISABLE_AVAILABILITY:"
make clean
CXXFLAGS="${CXXFLAGS/-D_LIBCPP_DISABLE_AVAILABILITY/}" make -j ${CPU_COUNT}

# Test 2: Check for any availability warnings or errors
echo "Checking for availability warnings or errors:"
CXXFLAGS="${CXXFLAGS/-D_LIBCPP_DISABLE_AVAILABILITY/}" make -j ${CPU_COUNT} 2>&1 | grep -i "availability"

# Test 3: Verify the installed files
echo "Verifying installed files:"
ls -l $PREFIX/bin/gw
ls -l $PREFIX/bin/.gw.ini

Consider documenting the reason for adding -D_LIBCPP_DISABLE_AVAILABILITY in a comment, explaining any specific compatibility issues it addresses.


Line range hint 14-24: Approve OS-specific flag settings with suggestion for macOS review.

The conditional flag settings for different OS types improve cross-platform support:

  1. Non-macOS systems now have appropriate SYSROOT_FLAGS, CPPFLAGS, and LDFLAGS set.
  2. MacOS has minimal settings, which might be intentional.

To ensure these changes are complete and functional, please run the following script:

#!/bin/bash
# Description: Verify OS-specific flag settings

# Test 1: Print out the values of the flags on different systems
echo "OS type: $OSTYPE"
echo "SYSROOT_FLAGS: $SYSROOT_FLAGS"
echo "CPPFLAGS: $CPPFLAGS"
echo "LDFLAGS: $LDFLAGS"

# Test 2: Verify the existence of the directories used in the flags
if [[ "$OSTYPE" != "darwin"* ]]; then
    echo "Checking non-macOS directories:"
    ls -l ${BUILD_PREFIX}/${HOST}/sysroot/usr/include
    ls -l ${BUILD_PREFIX}/${HOST}/sysroot/usr/lib
    ls -l ${BUILD_PREFIX}/${HOST}/sysroot/usr/lib64
else
    echo "Checking macOS directories:"
    ls -l ${PREFIX}/lib
fi

Consider reviewing if the minimal settings for macOS are sufficient or if additional flags are needed for complete functionality.


5-6: Verify the impact of USE_GL removal and make prep output change.

  1. The USE_GL=1 assignment has been commented out. Ensure this doesn't negatively impact the build process or runtime behavior.
  2. The make prep command now only redirects stderr to /dev/null, allowing stdout to be visible. This change improves visibility into the preparation process, but consider if any sensitive information might be exposed in stdout.

To check the impact of these changes, run the following script:

Consider capturing and reviewing the make prep output to ensure it doesn't contain sensitive information before allowing it to be visible.


Line range hint 9-11: Approve platform-specific changes with cross-platform compatibility check.

The modifications for non-macOS systems look appropriate:

  1. Adding -lGL -lGLX to the linker flags should provide necessary OpenGL support.
  2. Changing to GLFW_NATIVE_CONTEXT_API might be required for compatibility on these systems.

To ensure these changes don't negatively impact cross-platform compatibility, please run the following script:

Ensure that these changes don't break the build or functionality on macOS or other supported platforms.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
recipes/gw/meta.yaml (1)

75-77: Consider optimizing test commands

The added test commands are useful for basic verification, but they could be improved:

  1. The ls -lh command alone doesn't provide much context about the package installation.
  2. The which gw and ls -lh $(which gw) commands could be combined for efficiency.

Consider the following suggestions:

- - ls -lh
- - which gw
- - ls -lh $(which gw)
+ - command -v gw >/dev/null 2>&1 || { echo >&2 "gw not found in PATH"; exit 1; }
+ - ls -lh $(command -v gw)
+ - gw --version

These changes will:

  1. Check if gw is in the PATH and exit with an error if it's not.
  2. Show details of the gw executable if found.
  3. Display the version of gw, which is a common practice for verifying correct installation.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between bd24904 and 82cc2f2.

📒 Files selected for processing (1)
  • recipes/gw/meta.yaml (2 hunks)
🧰 Additional context used
🪛 yamllint
recipes/gw/meta.yaml

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)

🔇 Additional comments (1)
recipes/gw/meta.yaml (1)

1-2: LGTM: Version and checksum updated correctly

The version has been correctly updated to 1.1.1, and the SHA256 checksum has been changed accordingly. These changes are consistent with the PR objective to update GW to v1.1.1.

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
recipes/gw/meta.yaml (1)

75-79: Approve new test commands and suggest cleanup.

The new test commands provide more comprehensive and efficient checks for the package installation. They verify the executable's presence, inspect its architecture, and check the version, which are all valuable for ensuring correct package deployment.

Consider removing the commented-out line 79 to keep the file clean:

- #- gw hg19 -b https://github.com/kcleal/gw/releases/download/v1.0.0/demo1.bam -r chr8:37047270-37055161 -n > test.png; ls -lh test.png

If this test is no longer needed, it's better to remove it entirely rather than keeping it as a comment.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 67cf319 and 3207241.

📒 Files selected for processing (1)
  • recipes/gw/meta.yaml (2 hunks)
🧰 Additional context used
🪛 yamllint
recipes/gw/meta.yaml

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)

🔇 Additional comments (2)
recipes/gw/meta.yaml (2)

1-2: LGTM: Version and SHA256 updates are correct.

The version has been correctly updated to 1.1.1, and the SHA256 checksum has been changed accordingly. These updates are consistent with the PR objective to update GW to v1.1.1.

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)


Line range hint 1-79: Summary: Approve PR for updating GW to v1.1.1

The changes in this PR successfully update the GW package to version 1.1.1. The key updates include:

  1. Version number and SHA256 checksum have been correctly updated.
  2. New test commands have been added to provide more comprehensive checks for the package installation.

These changes align with the PR objectives and improve the package validation process. The update appears to be correctly implemented and ready for merging.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
recipes/gw/meta.yaml (1)

75-78: Improved test commands. Consider keeping the image generation test.

The new test commands provide more comprehensive checks for the installation and functionality of the gw package. These tests are more robust and less likely to fail due to environmental factors.

Consider keeping the image generation test as an optional or extended test. It could be valuable for visual verification. You might want to add it back with a condition or as a separate test step:

- gw --version
- if [[ "${CONDA_BUILD_CROSS_COMPILATION:-}" != "1" ]]; then
    gw hg19 -b https://github.com/kcleal/gw/releases/download/v1.0.0/demo1.bam -r chr8:37047270-37055161 -n > test.png && ls -lh test.png;
  fi

This way, the image generation test will only run when not cross-compiling, avoiding potential issues in certain build environments.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3207241 and a8c3d12.

📒 Files selected for processing (1)
  • recipes/gw/meta.yaml (2 hunks)
🧰 Additional context used
🪛 yamllint
recipes/gw/meta.yaml

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)

🔇 Additional comments (1)
recipes/gw/meta.yaml (1)

1-2: LGTM! Verify SHA256 checksum.

The version update to 1.1.1 is consistent with the PR objective. The SHA256 checksum has been updated accordingly.

To ensure the integrity of the package, please verify the SHA256 checksum using the following script:

🧰 Tools
🪛 yamllint

[error] 1-1: syntax error: found character '%' that cannot start any token

(syntax)

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