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

Fix testing #566

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

UnitedMarsupials
Copy link

The first commit makes usage of libtest_static conditional on whether we're building that executable.

The second commit gets rid of unnecessary bash-isms making the scripts usable with a regular sh (such as that found on BSD-systems).

I do not know, why changes to the processing of positional arguments is necessary -- but they are. The scripts are never passed the path to the just-built flif-executable.

Otherwise tests attempt to use libtest_static-executable unconditionally,
and fail, because it may not exist.
ziemek99
ziemek99 previously approved these changes Jan 9, 2024
@ziemek99 ziemek99 dismissed their stale review January 9, 2024 19:10

After taking a closer look I couldn't understand one of the changes.

IN=$2
OUTF=$3
OUTP=$4
FLIF=./flif
Copy link
Member

Choose a reason for hiding this comment

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

By using ./ the script will look for the binary in the same directory as itself, which is tools. That's not the case here – gitignore gives us a hint:

# binarys
src/flif

So by default you'll find the binary in src folder, not tools. I looked into my local copy (which I compiled previously) and sure enough, there it was. Hardcoding a path to a binary makes it impossible to use this script if the binary isn't stored there. If someone installed FLIF in their system by (sudo) make install, then the binary will be somewhere in system directories, so ./flif won't work in this case, too.

FLIF=$1
IN=$2
OUTF=$3
FLIF=./flif
Copy link
Member

Choose a reason for hiding this comment

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

Same as above – this shouldn't be hardcoded, especially not in this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants