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

adding RDEVAL #52070

Merged
merged 32 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
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
13 changes: 13 additions & 0 deletions recipes/rdeval/build.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#!/usr/bin/env bash

set -o errexit
set -o nounset
set -o xtrace

cd "$SRC_DIR"

export CXXFLAGS="$CXXFLAGS -O3 -I$PREFIX/include"
export LDFLAGS="$LDFLAGS -L$PREFIX/lib"

make -j"${CPU_COUNT}" CXX="${CXX}" CXXFLAGS="${CXXFLAGS}" LDFLAGS="${LDFLAGS}"
install -v -m 0755 build/bin/rdeval "$PREFIX/bin/rdeval"
38 changes: 38 additions & 0 deletions recipes/rdeval/foo.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
diff --git a/Makefile b/Makefile
index 5823f28..6d448f3 100644
--- a/Makefile
+++ b/Makefile
@@ -1,5 +1,5 @@
-CXX ?= g++
-INCLUDE_DIR = -I./include -I./gfalibs/include
+CXX ?= ${CXX}
+INCLUDE_DIR = -I${PREFIX}/include -I./include -I./gfalibs/include
Comment on lines +6 to +9
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

🛠️ Refactor suggestion

Verify the existence and necessity of the ./gfalibs/include directory

The ./gfalibs/include directory was not found, which may lead to build issues. Please ensure that this path is correct and that the necessary directories are present in the repository.

🔗 Analysis chain

Review include paths for conda build environment compatibility

While using ${CXX} and adding ${PREFIX}/include aligns with conda build practices, the relative paths ./include and ./gfalibs/include might cause issues in the conda build environment where the source directory structure could differ.

Consider using absolute paths or environment variables for all include paths:

-INCLUDE_DIR = -I${PREFIX}/include -I./include -I./gfalibs/include
+INCLUDE_DIR = -I${PREFIX}/include -I${SRC_DIR}/include -I${SRC_DIR}/gfalibs/include
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the include directories exist in the expected locations
echo "Checking for include directories..."
fd -t d "include$"
fd -t d "gfalibs"

Length of output: 119

WARNINGS = -Wall -Wextra

CXXFLAGS = -g -std=gnu++14 -O3 $(INCLUDE_DIR) $(WARNINGS)
@@ -12,8 +12,8 @@ SOURCE = src
INCLUDE = include
BINDIR := $(BUILD)/.o

-LDFLAGS := -pthread
-LIBS = -lz
+LDFLAGS := -pthread -L${PREFIX}/lib
+LIBS := -lz -lpthread
Comment on lines +17 to +20
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

Fix duplicate pthread linking and variable assignment

There are two issues in the linker configuration:

  1. -pthread appears both in LDFLAGS and LIBS, which is redundant
  2. Using := for LIBS might override important conda build environment variables

Apply this fix:

-LDFLAGS := -pthread -L${PREFIX}/lib
-LIBS := -lz -lpthread
+LDFLAGS = -L${PREFIX}/lib
+LIBS = -lz -pthread
📝 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
-LDFLAGS := -pthread
-LIBS = -lz
+LDFLAGS := -pthread -L${PREFIX}/lib
+LIBS := -lz -lpthread
LDFLAGS = -L${PREFIX}/lib
LIBS = -lz -pthread


OBJS := main input reads
BINS := $(addprefix $(BINDIR)/, $(OBJS))
@@ -22,12 +22,12 @@ BINS := $(addprefix $(BINDIR)/, $(OBJS))
GFALIBS_DIR := $(CURDIR)/gfalibs

head: $(BINS) gfalibs | $(BUILD)
- $(CXX) $(CXXFLAGS) $(LDFLAGS) -o $(BUILD)/$(TARGET) $(BINDIR)/* $(GFALIBS_DIR)/*.o $(LIBS)
+ $(CXX) $(CXXFLAGS) $(INCLUDE_DIR) $(LDFLAGS) -o $(BUILD)/$(TARGET) $(BINDIR)/* $(GFALIBS_DIR)/*.o $(LIBS)
Comment on lines +27 to +29
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

Fix compilation and linking command issues

There are several issues with the compilation commands:

  1. Include paths ($(INCLUDE_DIR)) aren't needed during linking
  2. Libraries ($(LIBS)) shouldn't be specified during object file compilation

Apply these fixes:

-	$(CXX) $(CXXFLAGS) $(INCLUDE_DIR) $(LDFLAGS) -o $(BUILD)/$(TARGET) $(BINDIR)/* $(GFALIBS_DIR)/*.o $(LIBS)
+	$(CXX) $(CXXFLAGS) $(LDFLAGS) -o $(BUILD)/$(TARGET) $(BINDIR)/* $(GFALIBS_DIR)/*.o $(LIBS)

-	$(CXX) $(CXXFLAGS) $(LDFLAGS) -c $(SOURCE)/$(notdir $@).cpp -o $@ $(LIBS)
+	$(CXX) $(CXXFLAGS) -c $(SOURCE)/$(notdir $@).cpp -o $@

Also applies to: 33-35


all: head validate regenerate

$(BINDIR)%: $(SOURCE)/%.cpp $(INCLUDE)/%.h | $(BINDIR)
- $(CXX) $(CXXFLAGS) $(LDFLAGS) -c $(SOURCE)/$(notdir $@).cpp -o $@
+ $(CXX) $(CXXFLAGS) $(LDFLAGS) -c $(SOURCE)/$(notdir $@).cpp -o $@ $(LIBS)

.PHONY: gfalibs
gfalibs:
30 changes: 30 additions & 0 deletions recipes/rdeval/gfalibs.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
diff --git a/gfalibs/Makefile b/gfalibs/Makefile
index de9ab9b..90a703c 100644
--- a/gfalibs/Makefile
+++ b/gfalibs/Makefile
@@ -1,5 +1,5 @@
-CXX = g++
-INCLUDE_DIR = -I./include
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

parallel-hashmap dependency not declared in meta.yaml.

The addition of parallel-hashmap include path suggests a dependency, but it's not declared in recipes/rdeval/meta.yaml. Please declare parallel-hashmap as a dependency.

🔗 Analysis chain

Verify parallel-hashmap dependency.

The addition of parallel-hashmap include path suggests a dependency. Ensure this dependency is properly declared in the recipe's meta.yaml.

Also applies to: 9-9

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if parallel-hashmap is declared as a dependency in meta.yaml

# Look for parallel-hashmap in meta.yaml
echo "Checking meta.yaml for parallel-hashmap dependency..."
rg -i "parallel-hashmap" "recipes/rdeval/meta.yaml"

# Check if parallel-hashmap headers exist in the include directory
echo "Checking for parallel-hashmap headers..."
fd -t f . "recipes/rdeval" -E "*.h" | rg -i "parallel-hashmap"

Length of output: 387


Script:

#!/bin/bash
# Description: Verify if parallel-hashmap is declared as a dependency in meta.yaml and check for its headers.

# Look for parallel-hashmap in meta.yaml
echo "Checking meta.yaml for parallel-hashmap dependency..."
rg -i "parallel-hashmap" "recipes/rdeval/meta.yaml"

# Check if parallel-hashmap headers exist in the include directory
echo "Checking for parallel-hashmap headers..."
rg -i "parallel-hashmap\.h" "recipes/rdeval/include/"

Length of output: 375


Script:

#!/bin/bash
# Description: Search the entire repository for any references to parallel-hashmap.

echo "Searching for 'parallel-hashmap' in the entire codebase..."
rg -i "parallel-hashmap"

Length of output: 2649

+CXX = $(CXX)
+INCLUDE_DIR = -I${PREFIX}/include -I./include -I./include/parallel-hashmap
WARNINGS = -Wall -Wextra

CXXFLAGS = -g -std=gnu++14 -O3 $(INCLUDE_DIR) $(WARNINGS)
@@ -8,7 +8,7 @@ TARGET = gfalibs
BUILD = build/bin
SOURCE = src
INCLUDE = include
-LDFLAGS :=
+LDFLAGS := -L${PREFIX}/lib -pthread

SOURCES = $(addsuffix .o, input-filters input-gfa input-agp gfa gfa-lines log stream-obj uid-generator struct output memory)

@@ -16,7 +16,7 @@ all: $(SOURCES)
@

%.o: $(SOURCE)/%.cpp $(INCLUDE)/%.h
- $(CXX) $(CXXFLAGS) $(LDFLAGS) -c $(SOURCE)/$(basename $@).cpp -o $@
+ $(CXX) $(CXXFLAGS) $(INCLUDE_DIR) $(LDFLAGS) -c $(SOURCE)/$(basename $@).cpp -o $@ -lz -lpthread

clean:
$(RM) *.o
45 changes: 45 additions & 0 deletions recipes/rdeval/meta.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
{% set name = "rdeval" %}
{% set version = "0.0.2" %}

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

source:
url: https://github.com/vgl-hub/{{ name }}/releases/download/v{{ version }}/{{ name }}.v{{ version }}-with_submodules.zip
sha256: 56c2f4d7d2c22b27d1b380dc41b81cc77a2d97c792a69b9360bed9e0027bac4d
patches:
- zlib.patch
- foo.patch
- gfalibs.patch

Comment on lines +8 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Patch files have not been submitted upstream

The patches zlib.patch, foo.patch, and gfalibs.patch have not been submitted upstream. Please verify their necessity and consider submitting them to the upstream repository to ensure maintainability.

🔗 Analysis chain

Verify patch files and their necessity

Three patch files are specified:

  • zlib.patch
  • foo.patch
  • gfalibs.patch

Please provide:

  1. A brief description of each patch's purpose in the recipe
  2. Confirmation that these patches have been submitted upstream

Further verify upstream submission of patch files

Please run the following script to check if the patches have been submitted upstream via pull requests or commits:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check patch contents and related upstream issues

# Check patch contents
for patch in zlib.patch foo.patch gfalibs.patch; do
  echo "=== Contents of $patch ==="
  if [ -f "recipes/rdeval/$patch" ]; then
    cat "recipes/rdeval/$patch"
  fi
done

# Check for related upstream issues
gh api repos/vgl-hub/rdeval/issues --jq '.[] | select(.title | contains("patch") or contains("build") or contains("zlib") or contains("gfalibs")) | {title: .title, url: .html_url}'

Length of output: 4190


Script:

#!/bin/bash
# Description: Check if patches have been submitted upstream via pull requests or commits

echo "=== Checking Pull Requests for patch submissions ==="
gh pr list --repo vgl-hub/rdeval --json title,url --jq '.[] | select(.title | test("zlib.patch|foo.patch|gfalibs.patch")) | {title: .title, url: .url}'

echo "=== Checking Commit Messages for patch references ==="
gh api repos/vgl-hub/rdeval/commits --paginate --jq '.[] | select(.commit.message | test("zlib.patch|foo.patch|gfalibs.patch")) | {message: .commit.message, url: .html_url}'

Length of output: 561

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

requirements:
build:
- {{ compiler('cxx') }}
- make
host:
- pthread-stubs
- zlib

test:
commands:
- rdeval --help
Comment on lines +30 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Binary rdeval not found in the codebase

The test command references rdeval, but no such binary exists. Please verify the correct binary name and update the test configuration accordingly.

🔗 Analysis chain

Verify binary name and enhance test coverage

The test command uses rdeval while a past review noted a different binary name genodsp. Please clarify:

  1. Which is the correct binary name?
  2. Are both binaries provided by this package?

Additionally, consider adding tests with sample data to verify core functionality.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check source code for binary names
rg -g 'Makefile*' -g '*.cpp' 'rdeval|genodsp'

# Look for example data files that could be used in tests
fd -g '*.fa' -g '*.fasta' -g '*.fastq'

Length of output: 448


Script:

#!/bin/bash
# Check source code for binary names
rg -g 'Makefile*' -g '*.cpp' -g '*.h' 'rdeval|genodsp'

# Look for example data files that could be used in tests
fd -e fa -e fasta -e fastq

Length of output: 915


about:
home: https://github.com/vgl-hub/{{ name }}
summary: A general purpose, multithreaded read analysis and manipulation tool.
license: MIT
license_family: MIT
license_file: LICENSE
dev_url: https://github.com/vgl-hub/{{ name }}
doc_url: https://github.com/vgl-hub/rdeval/blob/v{{ version }}/README.md

extra:
additional-platforms:
- linux-aarch64
- osx-arm64
52 changes: 52 additions & 0 deletions recipes/rdeval/zlib.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
diff --git a/gfalibs/include/output.h b/gfalibs/include/output.h
index aabbec0..20f0dc1 100644
--- a/gfalibs/include/output.h
+++ b/gfalibs/include/output.h
@@ -12,7 +12,7 @@
#include "gfa-lines.h"
#include "gfa.h"

-#include "zlib.h"
+#include <zlib.h>
#include "zstream/zstream_common.hpp"
#include "zstream/ozstream.hpp"
#include "zstream/ozstream_impl.hpp"
diff --git a/gfalibs/include/stream-obj.h b/gfalibs/include/stream-obj.h
index 1faa4df..8e28bf6 100644
--- a/gfalibs/include/stream-obj.h
+++ b/gfalibs/include/stream-obj.h
@@ -2,7 +2,7 @@
#define STREAM_OBJ_H

#include <fstream>
-#include "zlib.h"
+#include <zlib.h>

class membuf : public std::streambuf {

diff --git a/gfalibs/src/stream-obj.cpp b/gfalibs/src/stream-obj.cpp
index e9694ee..c6eca26 100644
--- a/gfalibs/src/stream-obj.cpp
+++ b/gfalibs/src/stream-obj.cpp
@@ -5,7 +5,7 @@

#include "bed.h"
#include "struct.h"
-#include "zlib.h"
+#include <zlib.h>
#include "global.h"
#include "log.h"
#include "threadpool.h"
diff --git a/src/reads.cpp b/src/reads.cpp
index 866f49b..ac714cf 100644
--- a/src/reads.cpp
+++ b/src/reads.cpp
@@ -14,7 +14,7 @@
#include "functions.h" // global functions
#include "stream-obj.h"

-#include "zlib.h"
+#include <zlib.h>
#include "zstream/zstream_common.hpp"
#include "zstream/ozstream.hpp"
#include "zstream/ozstream_impl.hpp"
Loading