-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Add tttrlib #51927
Add tttrlib #51927
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces significant updates to the Possibly related PRs
Suggested labels
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (3)
recipes/tttrlib/app_wrapper.c (3)
24-28
: Check formalloc
failure inget_new_full_path
functionThe
malloc
call may returnNULL
if memory allocation fails, which can lead to undefined behavior or a crash. It's good practice to check the result ofmalloc
before using the allocated memory.Apply this diff to add error handling:
char *newpath = malloc(strlen(topdir) + strlen(fname) + 2); +if (newpath == NULL) { + fprintf(stderr, "Memory allocation failed in get_new_full_path\n"); + exit(1); +} strcpy(newpath, topdir);
79-88
: Check formalloc
failure inmake_python_parameters
functionThe
malloc
call may fail, leading to aNULL
pointer dereference when usingstrcpy
andstrcat
. It's important to check ifparam
isNULL
before proceeding.Apply this diff to add error handling:
char *param = malloc(strlen(orig_param) + strlen(binary) + 4); +if (param == NULL) { + fprintf(stderr, "Memory allocation failed in make_python_parameters\n"); + exit(1); +} strcpy(param, "\"");
161-170
: Remove unused variableis_python
The variable
is_python
is declared but never used in themain
function. Removing it will clean up the code.Apply this diff to remove the unused variable:
-int is_python;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
recipes/tttrlib/LICENSE.txt
(1 hunks)recipes/tttrlib/app_wrapper.c
(1 hunks)recipes/tttrlib/build.sh
(1 hunks)recipes/tttrlib/meta.yaml
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- recipes/tttrlib/LICENSE.txt
🧰 Additional context used
🪛 Shellcheck
recipes/tttrlib/build.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 1-1: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 3-3: target_platform is referenced but not assigned.
(SC2154)
[warning] 16-16: Quote this to prevent word splitting.
(SC2046)
[warning] 25-25: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🪛 yamllint
recipes/tttrlib/meta.yaml
[error] 55-55: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
recipes/tttrlib/build.sh (2)
19-22
: LGTM! Well-implemented fallback mechanism.
The build fallback strategy with clear documentation is a good approach for handling memory-constrained environments.
3-6
: Verify target_platform environment variable.
While the platform-specific handling is correct, we should ensure the environment variable is properly set.
✅ Verification successful
target_platform
variable usage is valid and common in conda-forge recipes
The verification shows that target_platform
is widely used across many build scripts in the repository (found in over 70 recipe build scripts), confirming this is a standard conda-forge build variable. The platform-specific handling in the tttrlib recipe follows this common pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if target_platform is set by conda-build
# Expected: Should find references to target_platform in conda-build configs
# Test: Search for target_platform in conda-build configuration
rg -l "target_platform" "${CONDA_PREFIX}/conda-bld/src_cache/"
Length of output: 107
Script:
#!/bin/bash
# Let's verify if target_platform is used in other conda-forge recipes
# and check conda-forge documentation references
# Search for target_platform usage in recipe files
fd "meta.yaml|build.sh" | xargs rg "target_platform" -l
# Check if there's any conda-forge documentation in the repository
fd "README|CONTRIBUTING" -e md | xargs rg "conda-forge|target_platform" -A 2 -B 2
Length of output: 2276
🧰 Tools
🪛 Shellcheck
[warning] 3-3: target_platform is referenced but not assigned.
(SC2154)
mkdir b2 && cd b2 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add shebang and improve directory handling robustness.
The script should handle potential failures in directory operations and be more explicit about the shell interpreter.
+#!/bin/bash
+set -euo pipefail
+
-mkdir b2 && cd b2
+build_dir="build"
+mkdir -p "${build_dir}" || exit 1
+cd "${build_dir}" || exit 1
📝 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.
mkdir b2 && cd b2 | |
#!/bin/bash | |
set -euo pipefail | |
build_dir="build" | |
mkdir -p "${build_dir}" || exit 1 | |
cd "${build_dir}" || exit 1 |
🧰 Tools
🪛 Shellcheck
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 1-1: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
recipes/tttrlib/build.sh
Outdated
cmake \ | ||
-DCMAKE_INSTALL_PREFIX="$PREFIX" \ | ||
-DCMAKE_PREFIX_PATH="$PREFIX" \ | ||
-DBUILD_PYTHON_INTERFACE=ON \ | ||
-DCMAKE_BUILD_TYPE=Release \ | ||
-DCMAKE_LIBRARY_OUTPUT_DIRECTORY="$SP_DIR" \ | ||
-DCMAKE_SWIG_OUTDIR="$SP_DIR" \ | ||
-DBUILD_LIBRARY=ON \ | ||
-DPYTHON_VERSION=$(python -c 'import platform; print(platform.python_version())')\ | ||
-G Ninja .. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Fix Python version extraction and review CMake options.
- The Python version command should be quoted to prevent word splitting.
- Consider if BUILD_LIBRARY is necessary when BUILD_PYTHON_INTERFACE is enabled.
cmake \
-DCMAKE_INSTALL_PREFIX="$PREFIX" \
-DCMAKE_PREFIX_PATH="$PREFIX" \
-DBUILD_PYTHON_INTERFACE=ON \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_LIBRARY_OUTPUT_DIRECTORY="$SP_DIR" \
-DCMAKE_SWIG_OUTDIR="$SP_DIR" \
- -DBUILD_LIBRARY=ON \
- -DPYTHON_VERSION=$(python -c 'import platform; print(platform.python_version())')\
+ -DPYTHON_VERSION="$(python -c 'import platform; print(platform.python_version())')" \
-G Ninja ..
📝 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.
cmake \ | |
-DCMAKE_INSTALL_PREFIX="$PREFIX" \ | |
-DCMAKE_PREFIX_PATH="$PREFIX" \ | |
-DBUILD_PYTHON_INTERFACE=ON \ | |
-DCMAKE_BUILD_TYPE=Release \ | |
-DCMAKE_LIBRARY_OUTPUT_DIRECTORY="$SP_DIR" \ | |
-DCMAKE_SWIG_OUTDIR="$SP_DIR" \ | |
-DBUILD_LIBRARY=ON \ | |
-DPYTHON_VERSION=$(python -c 'import platform; print(platform.python_version())')\ | |
-G Ninja .. | |
cmake \ | |
-DCMAKE_INSTALL_PREFIX="$PREFIX" \ | |
-DCMAKE_PREFIX_PATH="$PREFIX" \ | |
-DBUILD_PYTHON_INTERFACE=ON \ | |
-DCMAKE_BUILD_TYPE=Release \ | |
-DCMAKE_LIBRARY_OUTPUT_DIRECTORY="$SP_DIR" \ | |
-DCMAKE_SWIG_OUTDIR="$SP_DIR" \ | |
-DPYTHON_VERSION="$(python -c 'import platform; print(platform.python_version())')" \ | |
-G Ninja .. |
🧰 Tools
🪛 Shellcheck
[warning] 16-16: Quote this to prevent word splitting.
(SC2046)
recipes/tttrlib/build.sh
Outdated
# Copy programs to bin | ||
cd $PREFIX/bin | ||
cp $SRC_DIR/bin/* . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve binary installation robustness.
The current binary installation process needs better error handling and validation.
-# Copy programs to bin
-cd $PREFIX/bin
-cp $SRC_DIR/bin/* .
+# Copy programs to bin
+cd "${PREFIX}/bin" || exit 1
+if [ -d "${SRC_DIR}/bin" ] && [ "$(ls -A "${SRC_DIR}/bin")" ]; then
+ cp "${SRC_DIR}"/bin/* . || exit 1
+else
+ echo "Warning: No binaries found in ${SRC_DIR}/bin"
+fi
📝 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.
# Copy programs to bin | |
cd $PREFIX/bin | |
cp $SRC_DIR/bin/* . | |
# Copy programs to bin | |
cd "${PREFIX}/bin" || exit 1 | |
if [ -d "${SRC_DIR}/bin" ] && [ "$(ls -A "${SRC_DIR}/bin")" ]; then | |
cp "${SRC_DIR}"/bin/* . || exit 1 | |
else | |
echo "Warning: No binaries found in ${SRC_DIR}/bin" | |
fi |
🧰 Tools
🪛 Shellcheck
[warning] 25-25: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
recipes/tttrlib/app_wrapper.c
Outdated
if (l > 4 && path[l - 4] == '.') { | ||
/* Remove extension */ | ||
path[l-4] = '\0'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjust condition to correctly handle extension removal
The condition l > 4
should be l >= 4
to correctly handle file names with extensions exactly four characters long.
Apply this diff to fix the condition:
-if (l > 4 && path[l - 4] == '.') {
+if (l >= 4 && path[l - 4] == '.') {
/* Remove extension */
path[l - 4] = '\0';
}
📝 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.
if (l > 4 && path[l - 4] == '.') { | |
/* Remove extension */ | |
path[l-4] = '\0'; | |
} | |
if (l >= 4 && path[l - 4] == '.') { | |
/* Remove extension */ | |
path[l-4] = '\0'; | |
} |
recipes/tttrlib/app_wrapper.c
Outdated
char path[MAX_PATH * 2]; | ||
size_t l; | ||
char *ch; | ||
DWORD ret = GetModuleFileName(NULL, path, MAX_PATH * 2); | ||
if (ret == 0) { | ||
fprintf(stderr, "Failed to get executable name, code %d\n", GetLastError()); | ||
exit(1); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle potential truncation in GetModuleFileName
If the executable's path exceeds the buffer size, GetModuleFileName
may truncate the path without indicating an error. This can lead to incorrect behavior when using the truncated path.
Apply this diff to properly handle truncation:
DWORD ret = GetModuleFileName(NULL, path, MAX_PATH * 2);
-if (ret == 0) {
+if (ret == 0 || ret == (MAX_PATH * 2)) {
fprintf(stderr, "Failed to get executable name or path was truncated, code %d\n", GetLastError());
exit(1);
}
📝 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.
char path[MAX_PATH * 2]; | |
size_t l; | |
char *ch; | |
DWORD ret = GetModuleFileName(NULL, path, MAX_PATH * 2); | |
if (ret == 0) { | |
fprintf(stderr, "Failed to get executable name, code %d\n", GetLastError()); | |
exit(1); | |
} | |
char path[MAX_PATH * 2]; | |
size_t l; | |
char *ch; | |
DWORD ret = GetModuleFileName(NULL, path, MAX_PATH * 2); | |
if (ret == 0 || ret == (MAX_PATH * 2)) { | |
fprintf(stderr, "Failed to get executable name or path was truncated, code %d\n", GetLastError()); | |
exit(1); | |
} |
recipes/tttrlib/app_wrapper.c
Outdated
{ | ||
SHELLEXECUTEINFO si; | ||
BOOL bResult; | ||
char *param, *python = NULL; | ||
param = strdup(GetCommandLine()); | ||
|
||
ZeroMemory(&si, sizeof(SHELLEXECUTEINFO)); | ||
si.cbSize = sizeof(SHELLEXECUTEINFO); | ||
/* Wait for the spawned process to finish, so that any output goes to the | ||
console *before* the next command prompt */ | ||
si.fMask = SEE_MASK_NO_CONSOLE | SEE_MASK_NOASYNC | SEE_MASK_NOCLOSEPROCESS; | ||
{ | ||
char *orig_param = param; | ||
python = get_python_binary(topdir); | ||
si.lpFile = python; | ||
param = make_python_parameters(find_param_start(param), binary); | ||
free(orig_param); | ||
si.lpParameters = param; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Secure handling of command-line parameters
Directly using GetCommandLine
can expose the application to command injection if parameters are not properly sanitized, especially when passed to ShellExecuteEx
.
Consider using CommandLineToArgvW
to safely parse command-line arguments and construct parameters securely.
-char *param, *python = NULL;
-param = strdup(GetCommandLine());
+LPWSTR *argvW;
+int argcW;
+argvW = CommandLineToArgvW(GetCommandLineW(), &argcW);
+if (argvW == NULL) {
+ fprintf(stderr, "Failed to parse command line arguments\n");
+ exit(1);
+}
+// Convert arguments back to ANSI or use wide-character versions throughout
This approach helps in correctly parsing arguments even if they contain spaces or special characters.
Committable suggestion skipped: line range outside the PR's diff.
recipes/tttrlib/app_wrapper.c
Outdated
char *param, *python = NULL; | ||
param = strdup(GetCommandLine()); | ||
|
||
ZeroMemory(&si, sizeof(SHELLEXECUTEINFO)); | ||
si.cbSize = sizeof(SHELLEXECUTEINFO); | ||
/* Wait for the spawned process to finish, so that any output goes to the | ||
console *before* the next command prompt */ | ||
si.fMask = SEE_MASK_NO_CONSOLE | SEE_MASK_NOASYNC | SEE_MASK_NOCLOSEPROCESS; | ||
{ | ||
char *orig_param = param; | ||
python = get_python_binary(topdir); | ||
si.lpFile = python; | ||
param = make_python_parameters(find_param_start(param), binary); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Prefer CreateProcess
over ShellExecuteEx
for enhanced control and security
Using CreateProcess
gives finer control over the execution environment and avoids potential security risks associated with ShellExecuteEx
, which may invoke the shell and unintentionally execute scripts or commands.
Apply this diff to switch to CreateProcess
:
BOOL bResult;
...
-bResult = ShellExecuteEx(&si);
+STARTUPINFO siStartupInfo;
+PROCESS_INFORMATION piProcessInfo;
+ZeroMemory(&siStartupInfo, sizeof(siStartupInfo));
+siStartupInfo.cb = sizeof(siStartupInfo);
+ZeroMemory(&piProcessInfo, sizeof(piProcessInfo));
+
+bResult = CreateProcess(
+ python, // lpApplicationName
+ param, // lpCommandLine
+ NULL, // lpProcessAttributes
+ NULL, // lpThreadAttributes
+ FALSE, // bInheritHandles
+ 0, // dwCreationFlags
+ NULL, // lpEnvironment
+ NULL, // lpCurrentDirectory
+ &siStartupInfo, // lpStartupInfo
+ &piProcessInfo // lpProcessInformation
+);
+if (bResult) {
+ WaitForSingleObject(piProcessInfo.hProcess, INFINITE);
+ GetExitCodeProcess(piProcessInfo.hProcess, &exit_code);
+ CloseHandle(piProcessInfo.hProcess);
+ CloseHandle(piProcessInfo.hThread);
+}
Committable suggestion skipped: line range outside the PR's diff.
recipes/tttrlib/app_wrapper.c
Outdated
char *python = malloc(strlen(topdir) + 12); | ||
strcpy(python, topdir); | ||
/* Remove last two components of the path (we are in Library/bin; python | ||
is in the top-level directory) */ | ||
remove_last_component(python); | ||
remove_last_component(python); | ||
strcat(python, "\\python.exe"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Validate existence of python.exe
in get_python_binary
Assuming the location of python.exe
without verification can lead to errors if the directory structure changes. It's advisable to check if the constructed path exists.
Apply this diff to add validation:
strcat(python, "\\python.exe");
+if (GetFileAttributes(python) == INVALID_FILE_ATTRIBUTES) {
+ fprintf(stderr, "Python executable not found at %s\n", python);
+ exit(1);
+}
return python;
📝 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.
char *python = malloc(strlen(topdir) + 12); | |
strcpy(python, topdir); | |
/* Remove last two components of the path (we are in Library/bin; python | |
is in the top-level directory) */ | |
remove_last_component(python); | |
remove_last_component(python); | |
strcat(python, "\\python.exe"); | |
char *python = malloc(strlen(topdir) + 12); | |
strcpy(python, topdir); | |
/* Remove last two components of the path (we are in Library/bin; python | |
is in the top-level directory) */ | |
remove_last_component(python); | |
remove_last_component(python); | |
strcat(python, "\\python.exe"); | |
if (GetFileAttributes(python) == INVALID_FILE_ATTRIBUTES) { | |
fprintf(stderr, "Python executable not found at %s\n", python); | |
exit(1); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/tttrlib/meta.yaml
(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/tttrlib/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (7)
recipes/tttrlib/meta.yaml (7)
1-7
: LGTM! Package metadata follows best practices.
The package name and version are properly defined using Jinja2 variables.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
12-14
: LGTM! Build configuration properly handles ABI compatibility.
The run_exports
with max_pin="x.x"
ensures proper ABI compatibility as required in the PR objectives.
50-61
: LGTM! Package metadata is complete and well-documented.
All required fields are present with appropriate URLs and license information.
25-25
: Verify SWIG version constraint rationale.
The SWIG version is pinned to <4.3.0
. Please document the reason for this constraint.
#!/bin/bash
# Description: Check if there are known compatibility issues with SWIG 4.3.0
# Expected: Find relevant discussions or issues
# Search for SWIG version discussions in the repository
rg -i "swig.*4\.3\.0"
63-66
: Verify maintainer permissions.
Let's verify if the maintainers have the necessary permissions and are active:
✅ Verification successful
Maintainers verified as active contributors
Both listed maintainers (tpeulen and khemmen) are confirmed as active contributors to the tttrlib repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if maintainers are active contributors
# Expected: Find contributions from listed maintainers
# Check if maintainers are contributors to the project
gh api -X GET repos/Fluorescence-Tools/tttrlib/contributors --jq '.[].login'
Length of output: 94
1-66
: Verify package suitability for Bioconda.
According to the PR objectives, new recipes not directly related to biological sciences should be submitted to conda-forge instead of Bioconda. Please clarify how this package is specifically used in biological sciences, particularly in fluorescence spectroscopy and microscopy applications.
✅ Verification successful
Package is suitable for Bioconda
Based on the search results, tttrlib is a library specifically designed for time-resolved fluorescence imaging and spectroscopic data analysis, which is directly relevant to biological research. The package is maintained by the Fluorescence-Tools organization and is used for:
- Processing time-resolved imaging data in biological microscopy
- Analyzing fluorescence spectroscopy data, which is a key technique in molecular and cell biology
- Supporting high-throughput biological imaging applications
The package's focus on fluorescence microscopy and spectroscopy, which are essential techniques in biological research, makes it appropriate for inclusion in Bioconda.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for biological science applications
# Expected: Find references to biological applications
# Search for biological science related terms in documentation
rg -i "biology|microscopy|fluorescence|spectroscopy" README.md docs/
Length of output: 116
Script:
#!/bin/bash
# Let's try searching in the repository root for any documentation files
fd -e md -e rst
# Also search the repository for biological/scientific terms in all text files
rg -i "biology|microscopy|fluorescence|spectroscopy|imaging|scientific"
Length of output: 55859
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
8-10
: Verify source integrity and accessibility.
The source URL and checksum look correct, but let's verify:
test: | ||
imports: | ||
- tttrlib | ||
commands: | ||
- tttrlib --help |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add click packages to test requirements.
The test command uses the CLI (tttrlib --help
) which depends on click packages, but they're not explicitly listed in test requirements.
test:
imports:
- tttrlib
+ requires:
+ - click
+ - click-didyoumean
commands:
- tttrlib --help
📝 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.
test: | |
imports: | |
- tttrlib | |
commands: | |
- tttrlib --help | |
test: | |
imports: | |
- tttrlib | |
requires: | |
- click | |
- click-didyoumean | |
commands: | |
- tttrlib --help |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
recipes/tttrlib/build.sh (2)
5-11
: Add platform validation and improve error handling.While
target_platform
is set by conda-build, we should validate it and handle unexpected cases.+# Validate target platform is set +if [[ -z "${target_platform:-}" ]]; then + echo "Error: target_platform is not set" >&2 + exit 1 +fi + if [[ "${target_platform}" == osx-* ]]; then # See https://conda-forge.org/docs/maintainer/knowledge_base.html#newer-c-features-with-old-sdk CXXFLAGS="${CXXFLAGS} -D_LIBCPP_DISABLE_AVAILABILITY" export CONFIG_ARGS="-DCMAKE_FIND_FRAMEWORK=NEVER -DCMAKE_FIND_APPBUNDLE=NEVER" else export CONFIG_ARGS="" fi🧰 Tools
🪛 Shellcheck
[warning] 5-5: target_platform is referenced but not assigned.
(SC2154)
23-26
: Enhance build process with better progress tracking and error handling.The fallback mechanism is good but could benefit from progress indication and better error detection.
-# On some platforms (notably aarch64 with Drone) builds can fail due to -# running out of memory. If this happens, try the build again; if it -# still fails, restrict to one core. -ninja install -k 0 || ninja install -k 0 || ninja install -j ${CPU_COUNT} +echo "Starting build with full parallelism..." +if ! ninja install -k 0; then + echo "Build failed, retrying..." + if ! ninja install -k 0; then + echo "Second attempt failed, trying with limited cores..." + if ! ninja install -j "${CPU_COUNT}"; then + echo "Build failed after all attempts" >&2 + exit 1 + fi + fi +firecipes/tttrlib/meta.yaml (2)
16-42
: Consider documenting SWIG version constraint.The SWIG version is pinned to <4.3.0. Please document the reason for this specific version constraint, as it might affect future maintenance.
53-56
: Fix typos in package description.There are typos in the description text that should be corrected.
- tttrlib is a simple, fast, libray to read, write and process - time-resolved imaging and spectroscopic data. For speed, it + tttrlib is a simple, fast library to read, write and process + time-resolved imaging and spectroscopic data. For speed, it
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/tttrlib/build.sh
(1 hunks)recipes/tttrlib/meta.yaml
(1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/tttrlib/build.sh
[warning] 3-3: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 5-5: target_platform is referenced but not assigned.
(SC2154)
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
🪛 yamllint
recipes/tttrlib/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (2)
recipes/tttrlib/meta.yaml (2)
12-14
: LGTM! Good use of run_exports.
The build configuration correctly implements run_exports as recommended in the PR objectives to prevent ABI breakage issues.
8-10
: Verify source tarball integrity.
The source URL and checksum format are correct. Let's verify the integrity of the source.
#!/bin/bash | ||
|
||
mkdir b2 && cd b2 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve build directory handling and error detection.
The build directory creation needs better error handling and cleanup.
#!/bin/bash
-
-mkdir b2 && cd b2
+set -euo pipefail
+
+BUILD_DIR="b2"
+trap 'rm -rf "${BUILD_DIR}"' EXIT
+rm -rf "${BUILD_DIR}"
+mkdir -p "${BUILD_DIR}" || exit 1
+cd "${BUILD_DIR}" || exit 1
📝 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.
#!/bin/bash | |
mkdir b2 && cd b2 | |
#!/bin/bash | |
set -euo pipefail | |
BUILD_DIR="b2" | |
trap 'rm -rf "${BUILD_DIR}"' EXIT | |
rm -rf "${BUILD_DIR}" | |
mkdir -p "${BUILD_DIR}" || exit 1 | |
cd "${BUILD_DIR}" || exit 1 |
🧰 Tools
🪛 Shellcheck
[warning] 3-3: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
recipes/tttrlib/build.sh
Outdated
cmake -S .. -B . \ | ||
-DCMAKE_CXX_COMPILER="${CXX}" \ | ||
-DCMAKE_INSTALL_PREFIX="$PREFIX" \ | ||
-DBUILD_PYTHON_INTERFACE=ON \ | ||
-DCMAKE_BUILD_TYPE=Release \ | ||
-DBUILD_LIBRARY=ON \ | ||
-DPYTHON_VERSION=$(python -c 'import platform; print(platform.python_version())') \ | ||
-G Ninja \ | ||
"${CONFIG_ARGS}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add missing CMake options and optimize build configuration.
The CMake configuration is missing important options for Python module installation.
cmake -S .. -B . \
-DCMAKE_CXX_COMPILER="${CXX}" \
-DCMAKE_INSTALL_PREFIX="$PREFIX" \
-DBUILD_PYTHON_INTERFACE=ON \
-DCMAKE_BUILD_TYPE=Release \
- -DBUILD_LIBRARY=ON \
-DPYTHON_VERSION="$(python -c 'import platform; print(platform.python_version())')" \
+ -DCMAKE_LIBRARY_OUTPUT_DIRECTORY="${SP_DIR}" \
+ -DCMAKE_SWIG_OUTDIR="${SP_DIR}" \
-G Ninja \
"${CONFIG_ARGS}"
📝 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.
cmake -S .. -B . \ | |
-DCMAKE_CXX_COMPILER="${CXX}" \ | |
-DCMAKE_INSTALL_PREFIX="$PREFIX" \ | |
-DBUILD_PYTHON_INTERFACE=ON \ | |
-DCMAKE_BUILD_TYPE=Release \ | |
-DBUILD_LIBRARY=ON \ | |
-DPYTHON_VERSION=$(python -c 'import platform; print(platform.python_version())') \ | |
-G Ninja \ | |
"${CONFIG_ARGS}" | |
cmake -S .. -B . \ | |
-DCMAKE_CXX_COMPILER="${CXX}" \ | |
-DCMAKE_INSTALL_PREFIX="$PREFIX" \ | |
-DBUILD_PYTHON_INTERFACE=ON \ | |
-DCMAKE_BUILD_TYPE=Release \ | |
-DPYTHON_VERSION="$(python -c 'import platform; print(platform.python_version())')" \ | |
-DCMAKE_LIBRARY_OUTPUT_DIRECTORY="${SP_DIR}" \ | |
-DCMAKE_SWIG_OUTDIR="${SP_DIR}" \ | |
-G Ninja \ | |
"${CONFIG_ARGS}" |
🧰 Tools
🪛 Shellcheck
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
# Copy programs to bin | ||
chmod 0755 $SRC_DIR/bin/* | ||
cp -f $SRC_DIR/bin/* $PREFIX/bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve security and robustness of binary installation.
The current binary installation process needs better security measures and validation.
-# Copy programs to bin
-chmod 0755 $SRC_DIR/bin/*
-cp -f $SRC_DIR/bin/* $PREFIX/bin
+# Install binaries with proper permissions
+BINDIR="${SRC_DIR}/bin"
+if [[ ! -d "${BINDIR}" ]]; then
+ echo "Error: Binary directory ${BINDIR} not found" >&2
+ exit 1
+fi
+
+# Ensure destination exists
+mkdir -p "${PREFIX}/bin"
+
+# Process each file individually with proper permission checks
+find "${BINDIR}" -type f -print0 | while IFS= read -r -d '' file; do
+ if [[ -x "${file}" ]]; then
+ install -v -m 755 "${file}" "${PREFIX}/bin/"
+ else
+ echo "Warning: Skipping non-executable file: ${file}" >&2
+ fi
+done
📝 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.
# Copy programs to bin | |
chmod 0755 $SRC_DIR/bin/* | |
cp -f $SRC_DIR/bin/* $PREFIX/bin | |
# Install binaries with proper permissions | |
BINDIR="${SRC_DIR}/bin" | |
if [[ ! -d "${BINDIR}" ]]; then | |
echo "Error: Binary directory ${BINDIR} not found" >&2 | |
exit 1 | |
fi | |
# Ensure destination exists | |
mkdir -p "${PREFIX}/bin" | |
# Process each file individually with proper permission checks | |
find "${BINDIR}" -type f -print0 | while IFS= read -r -d '' file; do | |
if [[ -x "${file}" ]]; then | |
install -v -m 755 "${file}" "${PREFIX}/bin/" | |
else | |
echo "Warning: Skipping non-executable file: ${file}" >&2 | |
fi | |
done |
bioconda does not build for windows -> remove windows app wrapper.
There was a problem hiding this 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 (2)
recipes/tttrlib/meta.yaml (2)
46-50
: Consider expanding test coverage.While basic import and CLI tests are present, consider adding more comprehensive tests for this scientific library:
- Test reading example data files
- Verify core functionality (e.g., data processing methods)
- Test different file format support
56-58
: Fix typos in package description.There are a few typos in the description text:
- tttrlib is a simple, fast, libray to read, write and process - time-resolved imaging and spectroscopic data. For speed, it + tttrlib is a simple, fast library to read, write and process + time-resolved imaging and spectroscopic data. For speed, it
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/tttrlib/meta.yaml
(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/tttrlib/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (4)
recipes/tttrlib/meta.yaml (4)
1-10
: LGTM! Package metadata is well-structured.
The package metadata section follows conda packaging best practices with proper templating and source verification.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
12-14
: LGTM! Build configuration properly handles ABI compatibility.
The run_exports
configuration with max_pin="x.x"
ensures proper ABI compatibility for downstream packages.
1-68
: LGTM! Recipe meets Bioconda guidelines.
The package is appropriately placed in Bioconda as it's relevant to biological sciences (fluorescence spectroscopy and microscopy). The recipe includes all required metadata and follows conda packaging best practices.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
25-25
: Verify SWIG version constraint.
The SWIG version is pinned to <4.3.0
. Please verify if this constraint is necessary or if a newer version could be supported.
✅ Verification successful
Let me search for more context about SWIG usage in the codebase.
Let me check for any SWIG-related build or interface files specifically.
Based on the search results, I can now provide a final response:
SWIG version constraint should be maintained for compatibility.
The package description explicitly states that tttrlib is "written in C++ and wrapped for Python via SWIG". The current SWIG version constraint (<4.3.0
) appears to be intentional, as other packages in the ecosystem that use SWIG for Python bindings (like rnablueprint, htseq) also specify version constraints. Without explicit testing of newer SWIG versions with this codebase, removing this constraint could lead to compatibility issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any known compatibility issues with newer SWIG versions
rg -i "swig.*4\.3" || echo "No explicit mentions of SWIG 4.3 compatibility issues found"
Length of output: 64
Script:
#!/bin/bash
# Check for SWIG interface files and any version-specific code
fd -e i -e swg . | xargs cat
# Look for build system files that might have SWIG-related configurations
fd -e cmake -e CMake . | xargs cat
# Check if there are any issues or discussions mentioning SWIG in documentation
fd README LICENSE CHANGELOG -t f | xargs cat
Length of output: 20336
Script:
#!/bin/bash
# Look for SWIG interface files in the repository
fd -e i -e swg -e swig . || echo "No SWIG interface files found"
# Check for any build-related files that might mention SWIG
rg -i "swig" --type cmake --type make --type yaml -A 3 -B 3 || echo "No SWIG-related build configurations found"
Length of output: 7644
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
recipes/tttrlib/meta.yaml (1)
55-58
: Fix typo in package descriptionThere's a typo in the description: "libray" should be "library".
- tttrlib is a simple, fast, libray to read, write and process + tttrlib is a simple, fast, library to read, write and process
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/tttrlib/meta.yaml
(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/tttrlib/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (5)
recipes/tttrlib/meta.yaml (5)
1-7
: LGTM: Package metadata is well-structured
The package name and version are properly templated using Jinja2 variables.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
12-14
: LGTM: Build configuration follows best practices
The run_exports configuration ensures proper version pinning for downstream dependencies.
46-50
: Previous comment about click packages still applies
65-68
: LGTM: Maintainer information is properly formatted
8-10
: Verify source URL accessibility and checksum
The source configuration looks good with proper version templating and checksum.
recipes/tttrlib/meta.yaml
Outdated
requirements: | ||
build: | ||
- {{ compiler('c') }} | ||
- {{ compiler('cxx') }} | ||
- pkg-config | ||
- cmake | ||
- ninja | ||
- make | ||
host: | ||
- swig <4.3.0 | ||
- doxygen | ||
- llvm-openmp # [osx] | ||
- libgomp # [linux] | ||
- boost-cpp | ||
- hdf5 | ||
- python | ||
- pip | ||
- numpy | ||
run: | ||
- python | ||
- llvm-openmp # [osx] | ||
- libgomp # [linux] | ||
- tqdm | ||
- click | ||
- click-didyoumean | ||
- {{ pin_compatible('numpy') }} | ||
- scikit-image | ||
- matplotlib-base | ||
- hdf5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add version constraints for critical dependencies
While the dependencies are well-organized, consider adding version constraints for critical components:
- Python version should be constrained to ensure compatibility
- boost-cpp and hdf5 versions should be pinned for stability
host:
- boost-cpp
- hdf5
- - python
+ - python >=3.8
- pip
- numpy
run:
- - python
+ - python >=3.8
- llvm-openmp # [osx]
Committable suggestion skipped: line range outside the PR's diff.
The error seems to be isolated in |
On my local Apple M1 setup (x86_64 Conda), the bioconda-utils build --git-range master --package tttrlib For example, here’s the output from the build with Python 3.12:
However, on the Bioconda build machine, CMake fails to locate MESSAGE(STATUS "Python version: '${PYTHON_VERSION}'")
cmake_policy(SET CMP0094 NEW)
FIND_PACKAGE(Python ${PYTHON_VERSION} EXACT COMPONENTS Interpreter Development.Module NumPy)
INCLUDE_DIRECTORIES(BEFORE ${Python_NumPy_PATH} ${Python_INCLUDE_DIRS})
LINK_LIBRARIES(${Python_LIBRARY} Python::NumPy) In the builds, The root cause is challenging to identify, possibly related to |
There was a problem hiding this 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 (3)
recipes/tttrlib/meta.yaml (3)
24-24
: Document SWIG version constraint rationaleThe SWIG version is pinned to
<4.3.0
. Please document the reason for this constraint in a comment, as it helps future maintenance.- - swig <4.3.0 + - swig <4.3.0 # Versions >=4.3.0 have compatibility issues with the build
29-33
: Consider pinning boost-cpp and hdf5 versionsFor better build reproducibility and to prevent potential ABI incompatibilities, consider pinning the versions of critical dependencies:
- boost-cpp - hdf5 - - python + - python >=3.8 - pip - numpy
55-58
: Fix typos in package descriptionThere are a few typos in the description text:
- tttrlib is a simple, fast, libray to read, write and process - time-resolved imaging and spectroscopic data. For speed, it + tttrlib is a simple, fast library to read, write and process + time-resolved imaging and spectroscopic data. For speed, it
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
recipes/tttrlib/meta.yaml
(1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/tttrlib/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (1)
recipes/tttrlib/meta.yaml (1)
8-10
: Verify source URL and checksum integrity
Let's ensure the source URL is accessible and the checksum matches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
recipes/tttrlib/build.sh (1)
24-27
: Enhance build failure handling and reporting.While the retry logic is good, it could benefit from better error reporting and exit code handling.
-ninja install -k 0 || ninja install -k 0 || ninja install -j ${CPU_COUNT} +# Function to attempt build with different strategies +build_with_strategy() { + local strategy="$1" + echo "Attempting build with strategy: $strategy" >&2 + case "$strategy" in + "default") + ninja install -k 0 + ;; + "retry") + ninja install -k 0 + ;; + "single-core") + ninja install -j "${CPU_COUNT:-1}" + ;; + esac + return $? +} + +for strategy in default retry single-core; do + if build_with_strategy "$strategy"; then + echo "Build successful with $strategy strategy" >&2 + exit 0 + else + echo "Build failed with $strategy strategy, trying next..." >&2 + fi +done + +echo "All build strategies failed" >&2 +exit 1recipes/tttrlib/meta.yaml (3)
12-15
: Monitor macOS Python >3.10 compatibility.The skip condition for macOS with Python >3.10 aligns with the reported build issues. Consider:
- Opening an upstream issue to track the macOS + Python >3.10 compatibility
- Adding a comment explaining the skip reason
build: number: 0 - skip: True # [osx and py > 310] + skip: True # [osx and py > 310] # CMake fails to locate Python_NumPy_INCLUDE_DIRS on macOS with Python >3.10 run_exports: '{{ pin_subpackage("tttrlib", max_pin="x.x") }}'
17-46
: Consider adding optional dependencies for imaging functionality.Given that the package supports time-resolved imaging data, consider adding optional dependencies like:
pillow
for image I/Oopencv
for advanced image processingWould you like me to help create a feature matrix to determine which dependencies should be optional vs. required?
57-60
: Fix typos in package description.- tttrlib is a simple, fast, libray to read, write and process - time-resolved imaging and spectroscopic data. For speed, it + tttrlib is a simple, fast library to read, write, and process + time-resolved imaging and spectroscopic data. For speed, it
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
recipes/tttrlib/build.sh
(1 hunks)recipes/tttrlib/meta.yaml
(1 hunks)
🧰 Additional context used
🪛 Shellcheck
recipes/tttrlib/build.sh
[warning] 3-3: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 5-5: target_platform is referenced but not assigned.
(SC2154)
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
🪛 yamllint
recipes/tttrlib/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (3)
recipes/tttrlib/meta.yaml (3)
1-6
: LGTM! Package metadata follows best practices.
The package name and version are properly defined using Jinja2 variables.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
67-70
: LGTM! Maintainers section is properly formatted.
8-10
: Verify source availability and checksum.
The source URL and checksum look correct, but let's verify:
-DCMAKE_BUILD_TYPE=Release \ | ||
-DBUILD_LIBRARY=ON \ | ||
-DPYTHON_VERSION=$(python -c 'import platform; print(platform.python_version())') \ | ||
-DPython_ROOT_DIR="${PREFIX}/bin" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix Python root directory path.
The Python root directory should point to PREFIX
, not PREFIX/bin
. The current path is incorrect and may cause Python component discovery issues.
- -DPython_ROOT_DIR="${PREFIX}/bin" \
+ -DPython_ROOT_DIR="${PREFIX}" \
📝 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.
-DPython_ROOT_DIR="${PREFIX}/bin" \ | |
-DPython_ROOT_DIR="${PREFIX}" \ |
ninja install -k 0 || ninja install -k 0 || ninja install -j ${CPU_COUNT} | ||
|
||
# Copy programs to bin | ||
chmod 0755 $SRC_DIR/bin/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add directory existence check before chmod.
The script attempts to modify permissions without verifying the existence of the directory or files, which could mask errors.
-chmod 0755 $SRC_DIR/bin/*
+if [[ -d "${SRC_DIR}/bin" ]] && compgen -G "${SRC_DIR}/bin/*" > /dev/null; then
+ chmod 0755 "${SRC_DIR}"/bin/*
+else
+ echo "Error: No files found in ${SRC_DIR}/bin" >&2
+ exit 1
+fi
📝 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.
chmod 0755 $SRC_DIR/bin/* | |
if [[ -d "${SRC_DIR}/bin" ]] && compgen -G "${SRC_DIR}/bin/*" > /dev/null; then | |
chmod 0755 "${SRC_DIR}"/bin/* | |
else | |
echo "Error: No files found in ${SRC_DIR}/bin" >&2 | |
exit 1 | |
fi |
if [[ "${target_platform}" == osx-* ]]; then | ||
# See https://conda-forge.org/docs/maintainer/knowledge_base.html#newer-c-features-with-old-sdk | ||
CXXFLAGS="${CXXFLAGS} -D_LIBCPP_DISABLE_AVAILABILITY" | ||
export CONFIG_ARGS="-DCMAKE_FIND_FRAMEWORK=NEVER -DCMAKE_FIND_APPBUNDLE=NEVER" | ||
else | ||
export CONFIG_ARGS="" | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add validation for target_platform variable.
The script assumes target_platform
is set but should validate its existence to prevent potential errors.
+if [[ -z "${target_platform:-}" ]]; then
+ echo "Error: target_platform is not set" >&2
+ exit 1
+fi
+
if [[ "${target_platform}" == osx-* ]]; then
# See https://conda-forge.org/docs/maintainer/knowledge_base.html#newer-c-features-with-old-sdk
CXXFLAGS="${CXXFLAGS} -D_LIBCPP_DISABLE_AVAILABILITY"
export CONFIG_ARGS="-DCMAKE_FIND_FRAMEWORK=NEVER -DCMAKE_FIND_APPBUNDLE=NEVER"
else
export CONFIG_ARGS=""
fi
📝 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.
if [[ "${target_platform}" == osx-* ]]; then | |
# See https://conda-forge.org/docs/maintainer/knowledge_base.html#newer-c-features-with-old-sdk | |
CXXFLAGS="${CXXFLAGS} -D_LIBCPP_DISABLE_AVAILABILITY" | |
export CONFIG_ARGS="-DCMAKE_FIND_FRAMEWORK=NEVER -DCMAKE_FIND_APPBUNDLE=NEVER" | |
else | |
export CONFIG_ARGS="" | |
fi | |
if [[ -z "${target_platform:-}" ]]; then | |
echo "Error: target_platform is not set" >&2 | |
exit 1 | |
fi | |
if [[ "${target_platform}" == osx-* ]]; then | |
# See https://conda-forge.org/docs/maintainer/knowledge_base.html#newer-c-features-with-old-sdk | |
CXXFLAGS="${CXXFLAGS} -D_LIBCPP_DISABLE_AVAILABILITY" | |
export CONFIG_ARGS="-DCMAKE_FIND_FRAMEWORK=NEVER -DCMAKE_FIND_APPBUNDLE=NEVER" | |
else | |
export CONFIG_ARGS="" | |
fi |
🧰 Tools
🪛 Shellcheck
[warning] 5-5: target_platform is referenced but not assigned.
(SC2154)
@BiocondaBot please fetch artifacts |
Package(s) built are ready for inspection:
|
I'll push this PR out in the mean time to make sure this tool is available for other users. Still not sure why |
@mencian thank you so much. That was really helpful. |
Please add tttrlib to bioconda.
tttrlib is a library to read and process time-resolved photon counting data widely used
in fluorescence spectroscopy and microscopy.
Please read the guidelines for Bioconda recipes before opening a pull request (PR).
General instructions
@BiocondaBot please add label
command.@bioconda/core
in 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_exports
section 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 eithername
if aname|lower
variable 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 label
please review & merge
label.@BiocondaBot please fetch artifacts
You can use this to test packages locally.
Note that the
@BiocondaBot please merge
command 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>
.