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.

Sign up for GitHub

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

Bump MUSCLE to 5.2 #51881

Merged
merged 2 commits into from
Nov 3, 2024
Merged

Bump MUSCLE to 5.2 #51881

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40aa501
Bump MUSCLE to 5.2
apcamargo Nov 2, 2024
22bbdee
Update recipes/muscle/build.sh
apcamargo Nov 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions recipes/muscle/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,18 @@
#!/bin/bash
set -e

cd src/
sed -i.bak -e 's/ -static//' Makefile
mkdir -p ${PREFIX}/bin
cd src || exit 1
echo "0" > gitver.txt

make CXX=$CXX
cp ${RECIPE_DIR}/vcxproj_make.py .
chmod +x vcxproj_make.py
./vcxproj_make.py --openmp --cppcompiler ${CXX}

mkdir -p "$PREFIX"/bin
cp "$(uname)"/muscle "$PREFIX"/bin/muscle
# Verify binary exists and is executable
if [ ! -f ../bin/muscle ]; then
apcamargo marked this conversation as resolved.
Show resolved Hide resolved
echo "Error: muscle binary not found"
exit 1
fi

cp ../bin/muscle ${PREFIX}/bin/muscle
10 changes: 5 additions & 5 deletions recipes/muscle/meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
{% set name = "muscle" %}
{% set version = "5.1.0" %}
{% set version = "5.2" %}

package:
name: {{ name|lower }}
version: {{ version }}

source:
url: https://github.com/rcedgar/muscle/archive/refs/tags/{{ version }}.tar.gz
sha256: 2bba8b06e3ccabf6465fa26f459763b2029d7e7b9596881063e3aaba60d9e87d
url: https://github.com/rcedgar/muscle/archive/refs/tags/v{{ version }}.tar.gz
sha256: 7aff1473241a36da94a88f37920a343fae91d2b6faae22e15c06591d1d0339b6
patches:
- support-linux-aarch64.patch

build:
number: 1
number: 0
run_exports:
- {{ pin_subpackage(name, max_pin="x.x") }}

Expand All @@ -30,7 +30,7 @@ about:
home: https://github.com/rcedgar/muscle
license: GPL-3.0-only
license_file: LICENSE
summary: "MUSCLE: multiple sequence alignment with high accuracy and high throughput"
summary: Multiple sequence and structure alignment with top benchmark scores scalable to thousands of sequences

extra:
additional-platforms:
Expand Down
14 changes: 7 additions & 7 deletions recipes/muscle/support-linux-aarch64.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
diff --git i/src/myutils.h w/src/myutils.h
index 49d28ef..8e8167e 100644
--- i/src/myutils.h
+++ w/src/myutils.h
@@ -11,7 +11,7 @@
#define TRACK_ALLOC 0
#define ALLOC_TOTALS 0
diff --git a/src/myutils.h b/src/myutils.h
index a5d8a58..fbf6f9f 100644
--- a/src/myutils.h
+++ b/src/myutils.h
@@ -10,7 +10,7 @@
#include <cstddef>
#include "myalloc.h"

-#if defined(__x86_64__) || defined(_M_X64) || defined(__arm64__)
+#if defined(__x86_64__) || defined(_M_X64) || defined(__arm64__) || defined(__aarch64__)
Expand Down
225 changes: 225 additions & 0 deletions recipes/muscle/vcxproj_make.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,225 @@
#!/usr/bin/env python

import argparse
import os
import sys

Usage = (
"Convert Visual Studio .vcxproj file in current directory to Makefile and run make."
)

AP = argparse.ArgumentParser(description=Usage)

# Value opts
AP.add_argument(
"--std",
required=False,
help="C++ standard option for GCC, e.g. c++11 or c++17 (default none)",
)
AP.add_argument(
"--cppcompiler",
required=False,
default="g++",
help="C++ compiler command name default g++)",
)
AP.add_argument(
"--ccompiler",
required=False,
default="gcc",
help="C++ compiler command name default gcc)",
)

# Flag opts
AP.add_argument("--debug", required=False, action="store_true", help="Debug build")
AP.add_argument("--openmp", required=False, action="store_true", help="Requires OMP")
AP.add_argument(
"--pthread", required=False, action="store_true", help="Requires pthread"
)
AP.add_argument("--lrt", required=False, action="store_true", help="Requires lrt")
AP.add_argument(
"--symbols",
required=False,
action="store_true",
help="Debug symbols (default if --debug)",
)
AP.add_argument(
"--nostrip",
required=False,
action="store_true",
help="Don't strip symbols (default if --debug or --symbols)",
)

Args = AP.parse_args()
debug = Args.debug
std = Args.std
cppcompiler = Args.cppcompiler
ccompiler = Args.ccompiler
nostrip = debug or Args.symbols
symbols = debug or Args.symbols

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)

Comment on lines +60 to +70
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add safeguards for project file handling.

The current implementation has potential issues:

  1. It silently uses the last found .vcxproj file if multiple exist
  2. 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.

Suggested change
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)

binary = ProjFileName.replace(".vcxproj", "")
sys.stderr.write("binary=" + binary + "\n")

compiler_opts = " -ffast-math -march=native"
linker_opts = " -ffast-math -march=native"
Comment on lines +74 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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"


if std:
compiler_opts += " --std=" + std

if debug:
compiler_opts += " -O0 -DDEBUG"
linker_opts += " -O0"
else:
compiler_opts += " -O3 -DNDEBUG"
linker_opts += " -O3"

if symbols:
compiler_opts += " -g3"
linker_opts += " -g3"

if Args.openmp:
compiler_opts += " -fopenmp"
linker_opts += " -fopenmp"

if Args.pthread:
compiler_opts += " -pthread"
linker_opts += " -lpthread"

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]

Comment on lines +99 to +113
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

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
Comment on lines +114 to +135
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.


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 $@ $<")

Comment on lines +137 to +218
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

sys.stderr.write("Makefile done.\n")

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")
Comment on lines +221 to +225
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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")

Loading