Closed schorsch3000 closed 4 months ago
Hmm indeed, on Linux the tests fail with OpenSCAD's nightly builds since 26 jan 2024, fix for that is in PR #6. That looks unrelated to the above though, which is a failure to properly split the r/g/b/a colors in the AMF merging code (whether anyone still needs AMF is another topic :sweat_smile:)
I would like to try reproducing the above failure, it works OK still for me. Could you give some info on your versions/environment? E.g. versions of colorscad, openscad, bash, which OS, etc. Maybe locale info is also interesting.
So, yeah that seems to be a nightly-build-problem.
Fun fact, i tend to run the a somewhat early nightly,
since opening this issue i updated to2024.03.28.ai1895
i can't reproduce my error now running 2024.03.28.ai1895
.
I Updated to 22024.04.05.ai18986
right now and All tests passed
as well.
So, according to me all is fine now, nothing to see here :-D
That said, are you Interested to reproduce anymore?
Anyway, Thanks!
Do you still know with which nightly appimage it failed? If so I'd like to have a shot at fixing it. I'm also setting up some workflows to run the tests automatically, so maybe that's already sufficient.
Anyway if it works now then all is good! Feel free to submit a PR with shellcheck fixes.
unfortunately i don't know, it must have been from April, but i dint know :-(
PR is coming up, I've done the easy part, sprinkling some quotes in there, end the script if bad things gonna happen, change some things to acknowledge best practice. Tests run fine on my setup.
3 things are missing for now:
the ambiguous until > "$NAME";
thing, yes, it works, yes, i understand what it does, but shellcheck minds, and i get it.
IFS=','; set -- $COLOR
seems like it's just missing quotes around $COLOR, but that breaks things, seems i need to dig in and find out what actually is happening here :-D
<(echo "$COLORS" | sed "s/\$/\.${FORMAT}/")
should be able be rewritten as a ${foo//search/replace}
.
My current state is here, if you are interested. I'll do the PR if everything is done.
Just a remark, CI will now run tests on all supported platforms so that should make it easier to verify your changes are OK. Might even consider adding a shellcheck step in the CI...
IFS=','; set -- $COLOR
This (and the followup) should probably be simplified as something like:
IFS=, read R G B A <<<"${COLOR//[\[\] ]/}"
There's probably more stuff like that, let's discuss it all on your PR.
adding shellcheck sounds like a great idea and should be quite easy i hope!
My first Task was to fix everything thatz is fixable without actually understanding the scope, just look at the few lines around shellcheck's finding and make improvements, that's easily done for missing quotes and alike, with the given tests im fairly confident everything is okay, for the next steps i actually need to check whats happening there :-D
But your hint will help.
To use the now added checks i will open a PR now, it's not done yet, but i like to utilize checks for osx and it's aged bash.
Hi, There are some issues in colorscad, mainly missing quotes around variables that make thing break with special chars (including space) in path names. I'm happy to fix at least everything that shellcheck complains about and make a PR for that.
It would be nice to have some tests to be sure everything i've done works, but the tests currently fail :-(
here is the full output of my testrun on a clean repo: