-
Notifications
You must be signed in to change notification settings - Fork 281
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
Remove EXIV2_EXT variable references #1289
Conversation
Thanks for looking. There is still work to be done. I tried to debug some python test code in Visual Studio Code last week and had trouble loading tests/system_tests.py because EXIV2_EXT wasn't defined. And we have to remove mention of it from the documentation. And there's always something we didn't know and didn't expect. |
Ah. This is a PR. I thought it was a comment on #1273. I'll ask @LeoHsiao1 to review as he has been involved with the python test code. I'll take care of update README.md (and possibly README-CONAN.md) when we believe the PR is ready to be merged. |
Already in this PR, so this should be it AFAIK ;) As for the CI failures, that's unfortunately beyond me... |
Ah, yes. I see you have worked on the README.md and README-CONAN.md Thanks. There is a reference to EXIV2_EXT in tests/suite.conf and I believe that's why Visual Studio Code refuses to launch/debug the test suite. Just removing that from tests/suite.conf will not solve that. We'll need to investigate the use of the variable
I need to investigate the CI (which I didn't implement). The builders frequently fail to download dependencies. And CodeCov often fails good builds for 100% incomprehensible reasons. MinGW/msys2 and Cygwin/64 don't get built by the CI. I have a macMini at home and use that to build/test the 8 supported platforms (macOS, Ubuntu, cygwin64, MinGW/msys2, msvc2019, solaris, NetBSD and FreeBSD). You know how it is. There's 100,000 lines of code to be maintained. I'm writing a book. And unbelievably I only have one pair hands. |
I have changed
Maybe |
I think it's causing problems because it is testing that some executables exist.
I'm working on something else at the moment. Let me finish what I'm doing, sync your code and see how this is works with Visual Studio Code. It's possible that everything's great and I'm talking about nothing. You've caught my by surprise by submitting a PR. I had asked @LeoHsiao1 to work on this and I thought you would test his PR on MinGW/msys2. @LeoHsiao1 has gone to the movies this evening. So, we working backwards. However this might work out perfectly if I conclude your PR is good, we wait for @LeoHsiao1 to review. |
Sorry, you assigned #1273 to me, and I understood I was thus asked to provide a PR... Thankfully the work is small so any duplication won't be significant... |
There's been a little misunderstanding. I think this is going to work out well. I'll update you later. Thanks for getting involved. Most people are very demanding while contributing nothing. You're the opposite. You're contributing something and demanding nothing. Thanks for working on this. |
@kmilos Apologies. I have run out of time today and will look at your PR tomorrow. This afternoon, I had to fix my macMini buildserver to build on all supported platforms. This was working perfectly at the end of June when I shipped 0.27.3. I've been working on my book in July and August. Now that I'm back working on the 0.27-maintenance branch, mysterious things have disturbed the environment on the macMini. I've fixed a cmake issue with Solaris. I totally rely on the macMini (and ignore the GitHub/CI). So.... tomorrow:
Apologies for the confusion about the assignment. I should have made you a "reviewer" and only assigned @LeoHsiao1 to the issue. So, you've "swapped seats". You remain assigned to this issue and @LeoHsiao1 is the reviewer. |
Mostly good news. I confirm that your code works well on MinGW/msys2, freebsd, netbsd, cygwin64, ubuntu and macOS. There's a build issue on Solaris which is unrelated to your code. 1290. Sadly msvc/Windows dies a horrible death which I'll investigate that today. I haven't investigated debugging tests/runner.py with Visual Studio Code. There's something causing a python traceback during I'm unhappy about the corpse of |
It does indeed stick out. However, it is set to '.exe' for Windows in system_tests.py, and it really is used to test the presence of required binaries. Maybe it can be fully internalized in |
It shouldn't be set to .exe in Windows. We remove corpses from the code. If something does nothing, it should not be there. Please remove it and update the PR. @LeoHsiao1 is to review the code and it's better that he contributes nothing to code that he reviews. |
I tried removing everything of course, the test for presence of binaries fails without '.exe' added on Windows. My suggestion is to merge the patch that is known to be working, and take another separate step. |
I will investigate the windows/.exe puzzle. |
It looks like you have to account for the extension on Windows, and there are suggestions to use the PATHEXT variable rather than assume and hard code anything. I also like the hint that one could skip searching for the file anyway, and catch the exception from Popen (which doesn't require the extension): https://stackoverflow.com/a/379535 |
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.
This removes the variable EXIV2_EXT
from the surface, but do we still need the variable binary_extension
?
Many suffixes are retained in the file exiv2/tests/ Suite.conf
:
[paths]
exiv2: ${ENV:exiv2_path}/exiv2${ENV:binary_extension}
exiv2json: ${ENV:exiv2_path}/exiv2json${ENV:binary_extension}
I had planned to improve both pieces of code to completely remove binary_extension
:
Line 166 in bb771e5
config['ENV']['binary_extension'] = config['ENV']['binary_extension'] or '.exe' |
Line 180 in bb771e5
if not os.path.exists(abs_path): |
The second commit removes any extension use and search for binaries, only relying on Popen to raise an error. Works or on mingw64. |
Oh, you did it while I was typing! |
Yesterday, I was complaining to Milos that I only have one pair of hands. Wrong! At least 3 pairs here, and two working on C++14 stuff on master. So great to have you both (@kmilos and @LeoHsiao1 ) working with me. Thank You. |
Unbelievable, Windows/MSVC passes. All I did was
I'm going to cut the grass in the garden and while I'm doing that, the macMini can build/test the supported platforms. After lunch I'll investigate Visual Studio Code on Windows. I don't expect an issue (mad fool). This smells good! |
Bon apetit! |
@kmilos Can you "sync" #1290 into your code-base please? It's solaris build fix. I can't remember what git/github calls "sync" - rebasing or something. Luis has done some good work on the CI on 'master' this morning. I hope we'll merge your stuff into 0.27-maintenance today. Tomorrow, I'll get the CI working better on 0.27-maintenance. |
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.
Thanks. Visual Studio Code is now debug tests/runner.py on Windows. I see the weirdo pythonic traceback from iotest is visible in Windows. I'll investigate that now. Then, I think we're done on this. I'll approve when I believe we're ready for the merge.
Great Job. Take the rest of the afternoon off!
I'm happy with this. I don't believe the pythonic traceback from tests/bash_tests/testcases.py/iotest() has anything to do with this PR. @LeoHsiao1 Can you review PR, please? If you're happy, approve and merge. @kmilos Thank You for working on this. Come back and see us again soon. When this PR has been merged, can you sync/test branch 0.27-maintenance and then close #1273 and #1269. |
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.
I tried the PR on Windows and it worked.
Thanks, pleasure to make a small contribution to your efforts. |
Addresses #1273
Tested ok only on mingw64 so far