Update FINEMAP to 1.4.2 #51978
Update FINEMAP to 1.4.2 #51978
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates to the Possibly related PRs
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
hopefully someone more knowledgeable than me will be able to figure out how to make osx work too The osx build is unable to find shared libraries. I tried using install_name_tool to replace the paths to the libraries, but I couldn't get it to work
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
recipes/finemap/meta.yaml (2)
9-12: Consider removing OSX source configuration.Since the build is skipped for non-Linux platforms (line 17), having the OSX source configuration is unnecessary and potentially confusing for maintainers.
source: url: http://www.christianbenner.com/finemap_v{{ version }}_x86_64.tgz # [linux] sha256: 3b1fc6eb3c2ccafd647b32e02d0244495cd0ade9ed7d474606c31ebf6e98b0c9 # [linux] - url: http://www.christianbenner.com/finemap_v{{ version }}_MacOSX.tgz # [osx] - sha256: e2967e5586347a8ac5cbcddf3c0e41bf8d3e7793875f78b509d07903d17a415b # [osx]
20-23: Remove unnecessary OSX installation command.Since the build is skipped for non-Linux platforms, the OSX installation command can be removed.
script: - mkdir -p ${PREFIX}/bin - install -m 755 finemap_v{{ version }}_x86_64 ${PREFIX}/bin/finemap # [linux] - - install -m 755 finemap_v{{ version }}_MacOSX ${PREFIX}/bin/finemap # [osx]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/finemap/meta.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/finemap/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (6)
recipes/finemap/meta.yaml (6)
1-7: LGTM! Package metadata follows best practices.
The use of Jinja2 templating for version and name variables is a good practice for maintainability.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
15-19: LGTM! Good addition of run_exports.
The addition of run_exports with max_pin="x" is a good practice to maintain ABI compatibility as mentioned in the PR guidelines.
26-29: LGTM! Requirements are properly specified.
The addition of the C++ compiler requirement is appropriate, and empty host/run sections are fine for this package.
32-33: LGTM! Improved test command.
The change from testing file existence to running --help is an improvement as it verifies basic program functionality.
39-41: LGTM! Improved YAML formatting.
The use of > for the multi-line summary improves YAML readability.
2-2: Verify version update compatibility.
Please ensure that version 1.4.2 maintains backward compatibility with 1.4.1.
✅ Verification successful
Based on the output from the shell scripts, I can generate the final response as we have clear information about the version update from the upstream website.
Version 1.4.2 is a safe, backward-compatible update
The update from 1.4.1 to 1.4.2 only includes bugfixes as per the upstream changelog (dated 27-Apr-2023), with no breaking changes mentioned. This type of update typically maintains backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any breaking changes in the new version
# Look for CHANGELOG or release notes in the repository
echo "Searching for changelog information..."
fd -i -g 'change*' -g 'release*' -g 'history*'
# Check upstream website for version information
echo "Checking upstream website for version details..."
curl -sL "http://www.christianbenner.com" | grep -A 5 "1.4.2"
Length of output: 1347
|
@BiocondaBot please add label I updated the recipe to work for linux, but I wasn't able to get it to work for osx too. The osx build is unable to find shared libraries. I tried using |
Thanks, Martin!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
recipes/finemap/meta.yaml (2)
9-12: Consider removing macOS source configurationSince the PR objectives mention unresolved macOS build issues and line 17 skips non-Linux builds, the macOS source configuration could be removed to avoid confusion. However, if you plan to fix macOS support in the future, it's acceptable to keep it commented out.
- url: http://www.christianbenner.com/finemap_v{{ version }}_x86_64.tgz # [linux] sha256: 3b1fc6eb3c2ccafd647b32e02d0244495cd0ade9ed7d474606c31ebf6e98b0c9 # [linux] - - url: http://www.christianbenner.com/finemap_v{{ version }}_MacOSX.tgz # [osx] - sha256: e2967e5586347a8ac5cbcddf3c0e41bf8d3e7793875f78b509d07903d17a415b # [osx]
21-23: Simplify installation scriptSince non-Linux builds are skipped, the macOS installation line can be removed.
script: - mkdir -p ${PREFIX}/bin - install -m 755 finemap_v{{ version }}_x86_64 ${PREFIX}/bin/finemap # [linux] - - install -m 755 finemap_v{{ version }}_MacOSX ${PREFIX}/bin/finemap # [osx]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/finemap/meta.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/finemap/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (5)
recipes/finemap/meta.yaml (5)
1-7: LGTM! Good use of Jinja2 templating
The package metadata is well-structured with proper templating variables.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
15-19: LGTM! Good use of run_exports
The run_exports configuration helps prevent ABI compatibility issues, which aligns with the PR guidelines.
31-32: LGTM! Improved test command
The new test command is better as it verifies that the binary can execute successfully.
38-39: LGTM! Better YAML formatting
Good use of the YAML block scalar > for the multi-line summary.
2-2: Verify version update compatibility
Please ensure that version 1.4.2 doesn't introduce breaking changes that would require additional configuration.
✅ Verification successful
Let me gather more information about this package and its version changes.
Based on the meta.yaml analysis, I can now generate the final response:
Version update appears safe to proceed
The version update to 1.4.2 appears safe as:
- Package has no dependencies that could be affected
- Binary-only package with simple installation
- No breaking changes in package structure
- Test command remains consistent (--help flag)
- Pin subpackage strategy remains appropriate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any changelog or release notes
rg -i "changelog|release.*notes" --type md
Length of output: 152
Script:
#!/bin/bash
# Check the meta.yaml file for any dependencies or build requirements
cat recipes/finemap/meta.yaml
# Look for any README or documentation specific to finemap
fd -t f "README|CHANGELOG" recipes/finemap/
Length of output: 1209
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
recipes/finemap/meta.yaml (1)
21-23: Consider removing unused macOS installation script.Since the build is skipped for non-linux64 platforms (line 17), the macOS installation script on line 23 will never be executed. Consider removing it to avoid confusion.
script: - mkdir -p ${PREFIX}/bin - install -m 755 finemap_v{{ version }}_x86_64 ${PREFIX}/bin/finemap # [linux] - - install -m 755 finemap_v{{ version }}_MacOSX ${PREFIX}/bin/finemap # [osx]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/finemap/meta.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/finemap/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (5)
recipes/finemap/meta.yaml (5)
1-7: LGTM! Version update and templating look good.
The version update to 1.4.2 matches the PR objectives, and the use of Jinja2 templating for version and name is a good practice.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
9-12: LGTM! Source configuration is properly structured.
The source configuration correctly:
- Uses platform-specific selectors
- Implements proper YAML list formatting for URLs and checksums
- Uses templated version variables
15-19: LGTM! Build configuration follows best practices.
The configuration correctly:
- Resets build number for new version
- Implements run_exports as per Bioconda guidelines
- Skips non-linux64 platforms due to known issues
27-27: LGTM! Improved test command.
The new test command finemap --help is more thorough than the previous file existence check as it verifies that the binary is properly installed and executable.
33-34: LGTM! Improved summary formatting.
The use of YAML block scalar indicator (>) improves the readability of the summary field while maintaining the same content.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
recipes/finemap/meta.yaml (2)
9-12: Ensure consistent selector usage.The selectors should be consistent throughout the file. Currently using both
linux64andnot linux64. Consider standardizing to one format.- - url: http://www.christianbenner.com/finemap_v{{ version }}_x86_64.tgz # [linux64] - sha256: 3b1fc6eb3c2ccafd647b32e02d0244495cd0ade9ed7d474606c31ebf6e98b0c9 # [linux64] + - url: http://www.christianbenner.com/finemap_v{{ version }}_x86_64.tgz # [linux] + sha256: 3b1fc6eb3c2ccafd647b32e02d0244495cd0ade9ed7d474606c31ebf6e98b0c9 # [linux]
22-24: Simplify installation script by removing skipped platform.Since the build is skipped for non-linux64 platforms (line 17), the MacOS installation command can be removed to avoid confusion.
script: - mkdir -p ${PREFIX}/bin - - install -m 755 finemap_v{{ version }}_x86_64 ${PREFIX}/bin/finemap # [linux64] - - install -m 755 finemap_v{{ version }}_MacOSX ${PREFIX}/bin/finemap # [osx] + - install -m 755 finemap_v{{ version }}_x86_64 ${PREFIX}/bin/finemap
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/finemap/LICENSE(1 hunks)recipes/finemap/meta.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- recipes/finemap/LICENSE
🧰 Additional context used
🪛 yamllint
recipes/finemap/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (2)
recipes/finemap/meta.yaml (2)
1-7: LGTM! Version update and templating look good.
The version update to 1.4.2 is correctly implemented using Jinja2 templating.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
27-28: LGTM! Improved test command.
The test command finemap --help is better than the previous file existence check as it verifies the binary is executable.
The OSX binary will not work. It needs to be relinked so that the shared library paths point to the conda versions. But relinking doesn't work because the binary was not built with the -headerpad_max_install_names option
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
recipes/finemap/meta.yaml (1)
16-17: Consider removing run_exports for binary packageThe
run_exportssection withmax_pin="x"might be unnecessarily strict for a binary package that doesn't provide shared libraries for linking. This is typically used for compiled libraries to ensure ABI compatibility.- run_exports: - - {{ pin_subpackage('finemap', max_pin="x") }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/finemap/meta.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/finemap/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (4)
recipes/finemap/meta.yaml (4)
1-7: LGTM: Package metadata is well-structured
The version update to 1.4.2 is properly implemented using Jinja2 templating.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
23-31: LGTM: Requirements are appropriate
The dependencies (libgomp, libgcc) are correctly specified for both build and run environments, which is necessary for the OpenMP-enabled binary.
51-57: LGTM: Lint exceptions are appropriate
The skip-lints are correctly configured for this binary package, addressing compiler usage, platform specificity, and style conventions.
33-39: Verify example data availability
The test configuration references example data, but we need to verify if this data is included in the downloaded binary package.
✅ Verification successful
Example data is present in the package
The verification confirms that the example data files are included in the downloaded binary package at finemap_v1.4.2_x86_64/example/. The package contains all necessary example files referenced in the test configuration:
data- main data filedata.z- z-scores filedata.ld- linkage disequilibrium filedata.k- configuration file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if example data is present in the source package
# Download and check the package contents
curl -sL "http://www.christianbenner.com/finemap_v1.4.2_x86_64.tgz" | tar tzf - | grep -i "example"
Length of output: 275
|
@BiocondaBot please fetch artifacts |
|
Package(s) built are ready for inspection:
|
|
@martin-g , this is ready for another review, if you're able 🙌 I removed the compiler requirement and replaced it with requirements for each of the dynamic shared libraries that conda-bld reported in the binary. I also added more thorough tests using the example data in the source. I decided to give up on the possibility of a MacOS build. As far as I can tell, it would require that the binary be rebuilt, but the source code isn't published. Lastly, I wanted to point out a few warnings that I didn't resolve:
I'm genuinely unsure of how to fix the first warning. And for the other two: I wasn't sure if it was safe to add those packages to |
| run: [] | ||
| build: | ||
| - {{ stdlib("c") }} | ||
| - libgomp |
There was a problem hiding this comment.
please use the compiler directive here.
There was a problem hiding this comment.
Hi @bgruening !
Is this what you're thinking?
| - libgomp | |
| - {{ compiler('cxx') }} | |
| - libgomp |
I actually had something like this in 8a0e784, but we decided to remove it in #51978 (comment), since I'm simply copying a binary and not compiling it. Shall I add it back?
There was a problem hiding this comment.
Yes this is what I was thinking. If @martin-g is ok with the way you have it now let's keep it.
Adding libgomp and libgcc will be done by the complier directive and it's a bit more portable. But you eitherway skip everything but linux64. So 🤷
There was a problem hiding this comment.
Ok, thanks for explaining!
I would like to make it work on macos eventually, so maybe it's a good idea to go with the more portable option for better future proofing. Or I can add a comment that explains to do that in the future.
I'll wait to hear thoughts from @martin-g
There was a problem hiding this comment.
I'll take a closer look later today!
There was a problem hiding this comment.
Do you need anything in the requirements > build section if you don't build it from source ?
There was a problem hiding this comment.
I tried that locally. The tests still seem to pass when I remove the requirements > build section, but conda-bld will issue a bunch of warnings.
WARNING (finemap,bin/finemap): /lib64/libdl.so.2 not found in packages, sysroot(s) nor the missing_dso_whitelist... is this binary repackaging?
WARNING (finemap,bin/finemap): /lib64/libm.so.6 not found in packages, sysroot(s) nor the missing_dso_whitelist... is this binary repackaging?
WARNING (finemap,bin/finemap): $RPATH/libgomp.so.1 not found in packages, sysroot(s) nor the missing_dso_whitelist... is this binary repackaging?
WARNING (finemap,bin/finemap): /lib64/libgcc_s.so.1 not found in packages, sysroot(s) nor the missing_dso_whitelist... is this binary repackaging?
WARNING (finemap,bin/finemap): /lib64/libpthread.so.0 not found in packages, sysroot(s) nor the missing_dso_whitelist... is this binary repackaging?
WARNING (finemap,bin/finemap): /lib64/libc.so.6 not found in packages, sysroot(s) nor the missing_dso_whitelist... is this binary repackaging?
WARNING (finemap,bin/finemap): /lib64/ld-linux-x86-64.so.2 not found in packages, sysroot(s) nor the missing_dso_whitelist... is this binary repackaging?
Should I just ignore those by adding the DSOs to missing_dso_whitelist? I was concerned that adding a package to missing_dso_whitelist might prevent conda-bld from automatically updating the path to the DSO in the binary (using patchelf) - since I couldn't find much info about the exact function of missing_dso_whitelist
There was a problem hiding this comment.
I don't have any experience with this kind of problems but I think you need to move those deps to requirements > host
There was a problem hiding this comment.
That makes sense. From what I've read online, shared libraries like those are supposed to be placed in the host section to ensure portability.
But when I move those requirements into the host section, I still get the same warnings mentioned above. So I guess I'll add them to missing_dso_whitelist
Also please feel free to let me know if there's somewhere else where I should also try to ask these questions. I've only recently been trying to learn how to create conda recipes, so I'm not very familiar with all of the resources yet.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/finemap/meta.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/finemap/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (6)
recipes/finemap/meta.yaml (6)
1-7: LGTM: Package metadata is well-structured
The version update and Jinja2 templating are implemented correctly.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
13-29: Build configuration looks good with documented platform limitations
The build configuration properly handles:
- Platform restrictions with clear documentation about macOS limitations
- Version pinning through run_exports
- DSO warnings through whitelist
Note: Future macOS support would require rebuilding with -headerpad_max_install_names option.
31-39: Requirements are properly structured for binary distribution
The host and run requirements correctly specify the necessary shared libraries.
41-48: Improved test coverage with functional tests
The test section now includes comprehensive functional tests using example data.
59-65: Extra configuration is properly documented
The lint skips are justified and well-documented. Maintainers list is up to date.
9-11: Verify source URL and checksum
The URL template and checksum look correct, but let's verify:
|
Thanks for merging and for all of the suggestions, @bgruening and @martin-g ! Agreed that this seemed very hacky. I can try to email the author next time and ask for the source code to be published. It would be much easier that way. |
|
That would be nice. Thanks @aryarm |
Update FINEMAP: 1.4.1 → 1.4.2
Please read the guidelines for Bioconda recipes before opening a pull request (PR).
General instructions
@BiocondaBot please add labelcommand.@bioconda/corein a comment.Instructions for avoiding API, ABI, and CLI breakage issues
Conda is able to record and lock (a.k.a. pin) dependency versions used at build time of other recipes.
This way, one can avoid that expectations of a downstream recipe with regards to API, ABI, or CLI are violated by later changes in the recipe.
If not already present in the meta.yaml, make sure to specify
run_exports(see here for the rationale and comprehensive explanation).Add a
run_exportssection like this:with
...being one of:{{ pin_subpackage("myrecipe", max_pin="x") }}{{ pin_subpackage("myrecipe", max_pin="x.x") }}{{ pin_subpackage("myrecipe", max_pin="x.x") }}(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin="x.x.x") }}(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin=None) }}while replacing
"myrecipe"with eithernameif aname|lowervariable is defined in your recipe or with the lowercase name of the package in quotes.Bot commands for PR management
Please use the following BiocondaBot commands:
Everyone has access to the following BiocondaBot commands, which can be given in a comment:
@BiocondaBot please update@BiocondaBot please add labelplease review & mergelabel.@BiocondaBot please fetch artifactsYou can use this to test packages locally.
Note that the
@BiocondaBot please mergecommand is now depreciated. Please just squash and merge instead.Also, the bot watches for comments from non-members that include
@bioconda/<team>and will automatically re-post them to notify the addressed<team>.