Bump MUSCLE to 5.2 #51881
Bump MUSCLE to 5.2 #51881
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files related to the "muscle" package. The Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
recipes/muscle/build.sh (3)
2-2: Consider enhancing error handling further.While adding
set -eis good, consider also addingset -uto catch undefined variables andset -o pipefailto ensure pipeline failures are caught.-set -e +set -euo pipefail
12-18: Enhance binary verification checks.While the existence check is good, consider enhancing the verification:
# Verify binary exists and is executable if [ ! -f ../bin/muscle ]; then - echo "Error: muscle binary not found" + echo "Error: muscle binary not found in ../bin/" exit 1 fi +if [ ! -x ../bin/muscle ]; then + echo "Error: muscle binary is not executable" + exit 1 +fi cp ../bin/muscle ${PREFIX}/bin/muscle
1-18: Consider cross-platform build implications.Given this is a Bioconda recipe and the build system has changed significantly:
- The switch to using Visual Studio project files (
vcxproj) for building might affect cross-platform compatibility. Ensure the build process works correctly on all supported platforms (Linux, macOS).- Consider adding platform-specific conditions in the build script if needed.
- Document the new build requirements in meta.yaml if vcxproj_make.py has additional dependencies.
recipes/muscle/vcxproj_make.py (1)
19-30: Fix typo and enhance compiler handling.The help text for
ccompilerhas a typo in "C++ compiler" (should be "C compiler"). Additionally, consider these improvements:
- Add compiler existence validation
- Support compiler override via environment variables (e.g.,
CC/CXX)AP.add_argument( "--ccompiler", required=False, - default="gcc", - help="C++ compiler command name default gcc)", + default=os.getenv("CC", "gcc"), + help="C compiler command name (default: gcc)", )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
recipes/muscle/build.sh(1 hunks)recipes/muscle/meta.yaml(2 hunks)recipes/muscle/support-linux-aarch64.patch(1 hunks)recipes/muscle/vcxproj_make.py(1 hunks)
🔇 Additional comments (7)
recipes/muscle/support-linux-aarch64.patch (1)
Line range hint 1-11: LGTM! Well-structured patch for AArch64 support.
The addition of __aarch64__ macro alongside __arm64__ is architecturally sound, ensuring proper 64-bit support across different ARM64 compiler implementations. The patch format is correct and the change is minimal yet effective.
Let's verify the patch format and context:
recipes/muscle/build.sh (2)
8-10: Verify the new build system implementation.
The build system has changed significantly from using make directly to using a Visual Studio project converter script. This is a substantial change that warrants verification:
- Ensure
vcxproj_make.pyis present in the recipe directory - Verify OpenMP support is properly configured
- Confirm the script handles the C++ compiler setting correctly
✅ Verification successful
Based on the provided output, I can now generate the final response since I have sufficient information to verify the build system implementation:
Build system changes are properly implemented and functional
The vcxproj_make.py script is present and implements a robust build system that:
- Correctly handles the OpenMP flag (
--openmp) by adding-fopenmpto both compiler and linker flags - Properly uses the specified C++ compiler from
${CXX}environment variable - Generates a Makefile with appropriate optimization flags and platform-specific settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the build script exists and is executable
if [ -f "recipes/muscle/vcxproj_make.py" ]; then
echo "Build script exists"
cat recipes/muscle/vcxproj_make.py
else
echo "Error: Build script missing"
fi
# Check for OpenMP related configurations
rg -l "openmp|OpenMP"
Length of output: 18843
4-6: Verify the purpose of gitver.txt.
The script creates a gitver.txt file with a hardcoded "0" value. Please confirm if this is the intended behavior for version 5.2 of MUSCLE.
✅ Verification successful
Let me search for more context about the version handling in MUSCLE to understand if this is the correct behavior.
Let me check the source code of MUSCLE to understand how version information is handled in the build process.
Let me check the patch file and the build script converter to understand if they have any relevance to version handling.
Let me check the source code repository to understand if this is a standard practice in MUSCLE builds.
Based on all the gathered information, I can now provide the final response:
The creation of gitver.txt with "0" value appears to be a build-time requirement
The gitver.txt file creation is specific to this conda-forge recipe's build process and is not directly related to MUSCLE's version tracking. The package version (5.2) is properly managed through conda-forge's meta.yaml, and the binary's version is verified in the test phase using muscle -version. The hardcoded "0" value is acceptable as it's just a build-time artifact.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if gitver.txt is used in the source code and how
rg -l "gitver.txt"
Length of output: 42
Script:
#!/bin/bash
# Search for version-related files and patterns in the source
rg -i "version|ver" recipes/muscle/ -A 3
# Check the meta.yaml for version information
cat recipes/muscle/meta.yaml
Length of output: 2369
Script:
#!/bin/bash
# Check the source repository for version/gitver related files
fd -t f -e cpp -e h . recipes/muscle/ | xargs rg -l "gitver"
# Look for Makefile or build configuration that might use gitver.txt
fd "Makefile|CMakeLists.txt|\.vcxproj" recipes/muscle/ | xargs cat
Length of output: 176
Script:
#!/bin/bash
# Check the patch file content
cat recipes/muscle/support-linux-aarch64.patch
# Check the vcxproj_make.py content
cat recipes/muscle/vcxproj_make.py
Length of output: 6030
Script:
#!/bin/bash
# Clone and check the upstream MUSCLE repository for version 5.2
gh repo view rcedgar/muscle --json description,url
# Check if there are any other references to gitver in the recipe
rg -C 3 "gitver" recipes/
Length of output: 1249
recipes/muscle/meta.yaml (4)
1-3: LGTM: Version update matches PR objectives
The version update to 5.2 is correctly specified and follows semantic versioning.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
14-18: LGTM: Build configuration follows best practices
- Build number correctly reset to 0 for new version
- The
run_exportssection withx.xpin properly handles API/ABI compatibility as per guidelines
32-34: LGTM: Improved package description
The updated summary provides a more comprehensive description of MUSCLE's capabilities, highlighting its key features (sequence/structure alignment) and performance characteristics (benchmark scores, scalability).
9-12: Verify source URL and checksum
The URL format change to include 'v' prefix is common for GitHub releases. Let's verify the URL and checksum.
| compiler_opts = " -ffast-math -march=native" | ||
| linker_opts = " -ffast-math -march=native" |
There was a problem hiding this comment.
Review compiler optimization flags for portability.
The hardcoded -march=native flag could cause issues in cross-compilation scenarios, particularly relevant for Bioconda packages that might need to support different architectures.
-compiler_opts = " -ffast-math -march=native"
-linker_opts = " -ffast-math -march=native"
+compiler_opts = " -ffast-math"
+linker_opts = " -ffast-math"
+# Only add native architecture optimizations if not cross-compiling
+if not os.getenv("CONDA_BUILD_CROSS_COMPILATION"):
+ compiler_opts += " -march=native"
+ linker_opts += " -march=native"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| compiler_opts = " -ffast-math -march=native" | |
| linker_opts = " -ffast-math -march=native" | |
| compiler_opts = " -ffast-math" | |
| linker_opts = " -ffast-math" | |
| # Only add native architecture optimizations if not cross-compiling | |
| if not os.getenv("CONDA_BUILD_CROSS_COMPILATION"): | |
| compiler_opts += " -march=native" | |
| linker_opts += " -march=native" |
| rc = os.system(r"rm -rf o/ ../bin/%s*" % binary) | ||
| if rc != 0: | ||
| sys.stderr.write("\n\nERROR -- failed to clean\n\n") | ||
| sys.exit(1) | ||
| sys.stderr.write("clean done.\n") | ||
|
|
||
| OBJDIR = "o" | ||
| BINDIR = "../bin" | ||
|
|
||
| Fields = ProjFileName.split("/") | ||
| n = len(Fields) | ||
| Name = Fields[n - 1] | ||
| Fields = Name.split(".") | ||
| binary = Fields[0] | ||
|
|
There was a problem hiding this comment.
Use safer methods for directory operations.
The current implementation uses unsafe shell commands and lacks proper error handling for directory operations.
-rc = os.system(r"rm -rf o/ ../bin/%s*" % binary)
-if rc != 0:
- sys.stderr.write("\n\nERROR -- failed to clean\n\n")
- sys.exit(1)
+import shutil
+
+def safe_remove_dir(path):
+ try:
+ if os.path.exists(path):
+ shutil.rmtree(path)
+ except OSError as e:
+ sys.stderr.write(f"\n\nERROR -- failed to clean {path}: {e}\n\n")
+ sys.exit(1)
+
+safe_remove_dir("o")
+safe_remove_dir(os.path.join("..", "bin"))Committable suggestion skipped: line range outside the PR's diff.
| with open("Makefile", "w") as f: | ||
|
|
||
| def Out(s): | ||
| print(s, file=f) | ||
|
|
||
| BINPATH = "$(BINDIR)/%s" % (binary) | ||
|
|
||
| Out("######################################################") | ||
| Out("# Makefile is generated by " + sys.argv[0]) | ||
| Out("# Don't edit the Makefile -- update the python script") | ||
| Out("######################################################") | ||
| Out("") | ||
| Out("BINDIR := %s" % BINDIR) | ||
| Out("OBJDIR := %s" % OBJDIR) | ||
| Out("BINPATH := %s" % BINPATH) | ||
|
|
||
| if CNames: | ||
| Out("") | ||
| Out("CC = " + ccompiler) | ||
| Out("CFLAGS := $(CFLAGS) " + compiler_opts) | ||
|
|
||
| if CXXNames: | ||
| Out("") | ||
| Out("CXX = " + cppcompiler) | ||
| Out("CXXFLAGS := $(CFLAGS) " + compiler_opts) | ||
|
|
||
| Out("") | ||
| Out("UNAME_S := $(shell uname -s)") | ||
| Out("LDFLAGS := $(LDFLAGS) " + linker_opts) | ||
| Out("ifeq ($(UNAME_S),Linux)") | ||
| Out(" LDFLAGS += -static") | ||
| Out("endif") | ||
|
|
||
| Out("") | ||
| Out("HDRS = \\") | ||
| for Name in sorted(HdrNames): | ||
| Out(" %s \\" % Name) | ||
|
|
||
| Out("") | ||
| Out("OBJS = \\") | ||
| for Name in CXXNames: | ||
| Out(" $(OBJDIR)/%s.o \\" % (Name)) | ||
|
|
||
| for Name in CNames: | ||
| Out(" $(OBJDIR)/%s.o \\" % (Name)) | ||
|
|
||
| Out("") | ||
| Out(".PHONY: clean") | ||
|
|
||
| Out("") | ||
| Out("$(BINPATH) : $(BINDIR)/ $(OBJDIR)/ $(OBJS)") | ||
|
|
||
| if len(CXXNames) > 0: | ||
| Cmd = "\t$(CXX) $(LDFLAGS) $(OBJS) -o $(BINPATH)" | ||
| else: | ||
| Cmd = "\t%(CC) $(LDFLAGS) $(OBJS) -o $(BINPATH)" | ||
|
|
||
| if Args.lrt: | ||
| Cmd += " -lrt" | ||
| Out(Cmd) | ||
|
|
||
| if not nostrip: | ||
| Out(" strip $(BINPATH)") | ||
|
|
||
| Out("") | ||
| Out("$(OBJDIR)/ :") | ||
| Out(" mkdir -p $(OBJDIR)/") | ||
|
|
||
| Out("") | ||
| Out("$(BINDIR)/ :") | ||
| Out(" mkdir -p $(BINDIR)/") | ||
|
|
||
| if CNames: | ||
| Out("") | ||
| Out("$(OBJDIR)/%.o : %.c $(HDRS)") | ||
| Out(" $(CC) $(CPPFLAGS) $(CFLAGS) -c -o $@ $<") | ||
|
|
||
| if CXXNames: | ||
| Out("") | ||
| Out("$(OBJDIR)/%.o : %.cpp $(HDRS)") | ||
| Out(" $(CXX) $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $<") | ||
|
|
There was a problem hiding this comment.
Enhance Makefile generation security and dependency tracking.
The current implementation has potential security issues with shell injection and could benefit from better dependency tracking.
- BINPATH = "$(BINDIR)/%s" % (binary)
+ BINPATH = "$(BINDIR)/%s" % (shlex.quote(binary))
+ # Add automatic dependency generation for better rebuild handling
+ Out(".PRECIOUS: $(OBJDIR)/%.d")
+ Out("$(OBJDIR)/%.d: %.cpp")
+ Out("\t@set -e; rm -f $@; \\")
+ Out('\t$(CXX) -MM $(CPPFLAGS) $< > $@.$$$$; \\')
+ Out('\tsed \'s,\\($*\\)\\.o[ :]*,$(OBJDIR)/\\1.o $@ : ,g\' < $@.$$$$ > $@; \\')
+ Out("\trm -f $@.$$$$")Committable suggestion skipped: line range outside the PR's diff.
| ProjFileName = None | ||
| HdrNames = [] | ||
| for FileName in os.listdir("."): | ||
| if FileName.endswith(".vcxproj"): | ||
| ProjFileName = FileName | ||
| elif FileName.endswith(".h"): | ||
| HdrNames.append(FileName) | ||
| if ProjFileName is None: | ||
| sys.stderr.write("\nProject file not found in current directory\n") | ||
| sys.exit(1) | ||
|
|
There was a problem hiding this comment.
Add safeguards for project file handling.
The current implementation has potential issues:
- It silently uses the last found .vcxproj file if multiple exist
- The file operations could be vulnerable to directory traversal
ProjFileName = None
HdrNames = []
for FileName in os.listdir("."):
+ # Prevent directory traversal
+ if os.path.dirname(FileName):
+ continue
if FileName.endswith(".vcxproj"):
+ if ProjFileName is not None:
+ sys.stderr.write("\nError: Multiple .vcxproj files found\n")
+ sys.exit(1)
ProjFileName = FileName
elif FileName.endswith(".h"):
+ if os.path.isfile(FileName): # Ensure it's a regular file
HdrNames.append(FileName)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ProjFileName = None | |
| HdrNames = [] | |
| for FileName in os.listdir("."): | |
| if FileName.endswith(".vcxproj"): | |
| ProjFileName = FileName | |
| elif FileName.endswith(".h"): | |
| HdrNames.append(FileName) | |
| if ProjFileName is None: | |
| sys.stderr.write("\nProject file not found in current directory\n") | |
| sys.exit(1) | |
| ProjFileName = None | |
| HdrNames = [] | |
| for FileName in os.listdir("."): | |
| # Prevent directory traversal | |
| if os.path.dirname(FileName): | |
| continue | |
| if FileName.endswith(".vcxproj"): | |
| if ProjFileName is not None: | |
| sys.stderr.write("\nError: Multiple .vcxproj files found\n") | |
| sys.exit(1) | |
| ProjFileName = FileName | |
| elif FileName.endswith(".h"): | |
| if os.path.isfile(FileName): # Ensure it's a regular file | |
| HdrNames.append(FileName) | |
| if ProjFileName is None: | |
| sys.stderr.write("\nProject file not found in current directory\n") | |
| sys.exit(1) |
| CXXNames = [] | ||
| CNames = [] | ||
| with open(ProjFileName) as File: | ||
| for Line in File: | ||
| Line = Line.strip() | ||
| Line = Line.replace('"', "") | ||
| Line = Line.replace(" ", "") | ||
| # <ClCompile Include="betadiv.cpp" /> | ||
| if Line.startswith("<ClCompileInclude"): | ||
| Fields = Line.split("=") | ||
| if len(Fields) != 2: | ||
| continue | ||
| FileName = Fields[1] | ||
| FileName = FileName.replace("/>", "") | ||
| if FileName.endswith(".cpp"): | ||
| FileName = FileName.replace(".cpp", "") | ||
| CXXNames.append(FileName) | ||
| elif FileName.endswith(".c"): | ||
| FileName = FileName.replace(".c", "") | ||
| CNames.append(FileName) | ||
|
|
||
| assert len(CXXNames) > 0 or len(CNames) > 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper XML parsing for .vcxproj files.
The current string-based XML parsing is fragile and could break with different .vcxproj formats. Additionally, source file existence should be validated.
+import xml.etree.ElementTree as ET
+
CXXNames = []
CNames = []
-with open(ProjFileName) as File:
- for Line in File:
- Line = Line.strip()
- Line = Line.replace('"', "")
- Line = Line.replace(" ", "")
- # <ClCompile Include="betadiv.cpp" />
- if Line.startswith("<ClCompileInclude"):
- Fields = Line.split("=")
- if len(Fields) != 2:
- continue
- FileName = Fields[1]
- FileName = FileName.replace("/>", "")
+try:
+ tree = ET.parse(ProjFileName)
+ root = tree.getroot()
+ for item in root.findall(".//ClCompile[@Include]"):
+ FileName = item.get("Include")
+ if not os.path.isfile(FileName):
+ sys.stderr.write(f"\nWarning: Source file {FileName} not found\n")
+ continue
if FileName.endswith(".cpp"):
FileName = FileName.replace(".cpp", "")
CXXNames.append(FileName)
elif FileName.endswith(".c"):
FileName = FileName.replace(".c", "")
CNames.append(FileName)
+except ET.ParseError as e:
+ sys.stderr.write(f"\nError parsing {ProjFileName}: {e}\n")
+ sys.exit(1)Committable suggestion skipped: line range outside the PR's diff.
| rc = os.system("make") | ||
| if rc != 0: | ||
| sys.stderr.write("\n\nERROR -- make failed\n\n") | ||
| sys.exit(1) | ||
| sys.stderr.write("make done.\n") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve build execution and error handling.
The current implementation doesn't capture build output and lacks proper signal handling.
+import subprocess
+import signal
+
+def signal_handler(signum, frame):
+ sys.stderr.write("\n\nInterrupted by signal %d\n" % signum)
+ sys.exit(1)
+
+signal.signal(signal.SIGINT, signal_handler)
+signal.signal(signal.SIGTERM, signal_handler)
+
-rc = os.system("make")
-if rc != 0:
- sys.stderr.write("\n\nERROR -- make failed\n\n")
+try:
+ process = subprocess.run(["make"], capture_output=True, text=True)
+ sys.stderr.write(process.stdout)
+ sys.stderr.write(process.stderr)
+ if process.returncode != 0:
+ sys.stderr.write("\n\nERROR -- make failed\n\n")
+ sys.exit(process.returncode)
+except subprocess.CalledProcessError as e:
+ sys.stderr.write("\n\nERROR -- make failed: %s\n\n" % str(e))
sys.exit(1)
-sys.stderr.write("make done.\n")
+except KeyboardInterrupt:
+ sys.stderr.write("\n\nInterrupted by user\n")
+ sys.exit(1)
+else:
+ sys.stderr.write("make done.\n")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rc = os.system("make") | |
| if rc != 0: | |
| sys.stderr.write("\n\nERROR -- make failed\n\n") | |
| sys.exit(1) | |
| sys.stderr.write("make done.\n") | |
| import subprocess | |
| import signal | |
| def signal_handler(signum, frame): | |
| sys.stderr.write("\n\nInterrupted by signal %d\n" % signum) | |
| sys.exit(1) | |
| signal.signal(signal.SIGINT, signal_handler) | |
| signal.signal(signal.SIGTERM, signal_handler) | |
| try: | |
| process = subprocess.run(["make"], capture_output=True, text=True) | |
| sys.stderr.write(process.stdout) | |
| sys.stderr.write(process.stderr) | |
| if process.returncode != 0: | |
| sys.stderr.write("\n\nERROR -- make failed\n\n") | |
| sys.exit(process.returncode) | |
| except subprocess.CalledProcessError as e: | |
| sys.stderr.write("\n\nERROR -- make failed: %s\n\n" % str(e)) | |
| sys.exit(1) | |
| except KeyboardInterrupt: | |
| sys.stderr.write("\n\nInterrupted by user\n") | |
| sys.exit(1) | |
| else: | |
| sys.stderr.write("make done.\n") |
|
@BiocondaBot please add label |
Co-authored-by: Martin Grigorov <[email protected]>
|
@apcamargo @martin-g Please see this thread: rcedgar/muscle#83 Something went wrong in this PR for osx64 |
|
@ori-scala answering the comment in the other issue, nobody "owns" the Bioconda releases. It's a community project where people dedicate their free time to maintain it. I don't have experience with ARM builds, so I'm not sure what is causing the problem here. We could just release a new build disabling the macOS ARM build for version 5.2. |
|
@apcamargo Thank you for the prompt reply! Truly appreciate it. I've also opened an issue to track this: #52034 |
|
I'm working on the new release. I added new tests and the ARM build seems to be working fine. However, the x64 build (that worked in the previous version) is now failing. See here: #52036 |
This PR updates
muscleto 5.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>.