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

Add tttrlib #51927

Merged
merged 20 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
27 changes: 27 additions & 0 deletions recipes/tttrlib/LICENSE.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
BSD-3-Clause license
Copyright (c) 2015-2022, conda-forge contributors
All rights reserved.

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are met:

1. Redistributions of source code must retain the above copyright notice,
this list of conditions and the following disclaimer.
2. Redistributions in binary form must reproduce the above copyright
notice, this list of conditions and the following disclaimer in the
documentation and/or other materials provided with the distribution.
3. Neither the name of the copyright holder nor the names of its
contributors may be used to endorse or promote products derived from
this software without specific prior written permission.

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE FOR
ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
DAMAGE.
175 changes: 175 additions & 0 deletions recipes/tttrlib/app_wrapper.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
#include <stdio.h>
#include <windows.h>
#include <string.h>
#include <stdlib.h>

/*
See: https://github.com/conda-forge/imp-feedstock/tree/main/recipe
IMP Python command line tools on Unix/Linux/Mac are typically named
without a .py extension (e.g. 'myapp' not 'myapp.py') and rely on a
#!/usr/bin/python line at the start of the file to tell the OS that
they are Python files. This doesn't work on Windows though, since
Windows relies on the file extension.

To remedy this problem, we provide this wrapper. Compile with
cl app_wrapper.c shell32.lib
then copy the resulting app_wrapper.exe to myapp.exe and install in
the top-level Anaconda directory. Then a user should be able to simply
type 'myapp', and the right Python tool will be run.
*/

/* Get full path to fname in topdir */
static char *get_new_full_path(const char *topdir, const char *fname)
{
char *newpath = malloc(strlen(topdir) + strlen(fname) + 2);
strcpy(newpath, topdir);
strcat(newpath, "\\");
strcat(newpath, fname);
return newpath;
}

/* Get full path to this binary */
static void get_full_path(char **dir, char **fname)
{
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);
}
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

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.

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

l = strlen(path);
if (l > 4 && path[l - 4] == '.') {
/* Remove extension */
path[l-4] = '\0';
}
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

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.

Suggested change
if (l > 4 && path[l - 4] == '.') {
/* Remove extension */
path[l-4] = '\0';
}
if (l >= 4 && path[l - 4] == '.') {
/* Remove extension */
path[l-4] = '\0';
}

ch = strrchr(path, '\\');
if (ch) {
*ch = '\0';
*fname = strdup(ch + 1);
} else {
*fname = strdup("");
}
*dir = strdup(path);
}

/* Find where the parameters start in the command line (skip past the
executable name) */
static char *find_param_start(char *cmdline)
{
BOOL in_quote = FALSE, in_space = FALSE;
for (; *cmdline; cmdline++) {
/* Ignore spaces that are quoted */
if (*cmdline == ' ' && !in_quote) {
in_space = TRUE;
} else if (*cmdline == '"') {
in_quote = !in_quote;
}
/* Return the first nonspace that follows a space */
if (in_space && *cmdline != ' ') {
break;
}
}
return cmdline;
}

/* Convert original parameters into those that python.exe wants (i.e. prepend
the name of the Python script) */
static char *make_python_parameters(const char *orig_param, const char *binary)
{
char *param = malloc(strlen(orig_param) + strlen(binary) + 4);
strcpy(param, "\"");
strcat(param, binary);
strcat(param, "\" ");
strcat(param, orig_param);
/*printf("python param %s\n", param);*/
return param;
}

/* Remove the last component of the path in place */
static void remove_last_component(char *dir)
{
char *pt;
if (!dir || !*dir) return;
/* Remove trailing slash if present */
if (dir[strlen(dir)-1] == '\\') {
dir[strlen(dir)-1] = '\0';
}
/* Remove everything after the last slash */
pt = strrchr(dir, '\\');
if (pt) {
*pt = '\0';
}
}

/* Get the full path to the Anaconda Python. */
static char* get_python_binary(const char *topdir)
{
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");
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

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.

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

return python;
}

/* Run the given Python script, passing it the parameters we ourselves
were given. */
static DWORD run_binary(const char *binary, const char *topdir)
{
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);
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

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.

free(orig_param);
si.lpParameters = param;
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

⚠️ Potential issue

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.

}
si.nShow = SW_SHOWNA;
bResult = ShellExecuteEx(&si);
free(param);
free(python);

if (bResult) {
if (si.hProcess) {
DWORD exit_code;
WaitForSingleObject(si.hProcess, INFINITE);
GetExitCodeProcess(si.hProcess, &exit_code);
CloseHandle(si.hProcess);
return exit_code;
}
} else {
fprintf(stderr, "Failed to start process, code %d\n", GetLastError());
exit(1);
}
return 0;
}

int main(int argc, char *argv[]) {
int is_python;
char *dir, *fname, *new_full_path;
DWORD return_code;

get_full_path(&dir, &fname);
/*printf("%s, %s\n", dir, fname);*/
new_full_path = get_new_full_path(dir, fname);
/*printf("new full path %s, %d\n", new_full_path, is_python);*/
return_code = run_binary(new_full_path, dir);
free(dir);
free(fname);
free(new_full_path);
return return_code;
}
26 changes: 26 additions & 0 deletions recipes/tttrlib/build.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
mkdir b2 && cd b2

Comment on lines +3 to +4
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

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.

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

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"
fi

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 ..
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 Python version extraction and review CMake options.

  1. The Python version command should be quoted to prevent word splitting.
  2. 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.

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


# 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 1

# Copy programs to bin
cd $PREFIX/bin
cp $SRC_DIR/bin/* .
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 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.

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

66 changes: 66 additions & 0 deletions recipes/tttrlib/meta.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
{% set name = "tttrlib" %}
{% set version = "0.25.0" %}

package:
name: {{ name }}
version: {{ version }}

source:
url: https://github.com/Fluorescence-Tools/tttrlib/archive/refs/tags/v{{ version }}.tar.gz
sha256: e15604ef063257cd87d3bdf266c1d5a448189508f51cdc4156a93cb2b46fe807

build:
number: 0
run_exports: '{{ pin_subpackage("tttrlib", max_pin="x.x") }}'

requirements:
build:
- {{ compiler('c') }}
- {{ compiler('cxx') }}
- pkg-config
- cmake
- ninja
- make # [linux]
- doxygen
- swig <4.3.0
host:
- llvm-openmp # [osx]
- libgomp # [linux]
- libboost-cpp
- hdf5
- python
- pip
- numpy
run:
- python
- tqdm
- click
- click-didyoumean
- {{ pin_compatible('numpy') }}
- scikit-image
- matplotlib-base
- hdf5

test:
imports:
- tttrlib
commands:
- tttrlib --help
Comment on lines +48 to +52
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

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.

Suggested change
test:
imports:
- tttrlib
commands:
- tttrlib --help
test:
imports:
- tttrlib
requires:
- click
- click-didyoumean
commands:
- tttrlib --help


about:
home: https://github.com/fluorescence-tools/tttrlib
summary: 'A file format agnostic library for time-resolved imaging and spectroscopic data.'
description: |
tttrlib is a simple, fast, libray to read, write and process
time-resolved imaging and spectroscopic data. For speed, it
is written in C++ and wrapped for Python via SWIG.
license: BSD-3-Clause
license_family: BSD
license_file: LICENSE.txt
doc_url: https://tttrlib.readthedocs.io
dev_url: https://github.com/fluorescence-tools/tttrlib

extra:
recipe-maintainers:
- tpeulen
- khemmen
Loading