linux-test-project / lcov

LCOV
GNU General Public License v2.0
867 stars 235 forks source link

use of system specific system calls like pwd and /dev/null break native Win32 support #215

Open GitMensch opened 1 year ago

GitMensch commented 1 year ago

Some commands use pwd: https://github.com/search?q=repo%3Alinux-test-project%2Flcov+pwd+language%3APerl&type=code&l=Perl and redirection to /dev/null: https://github.com/search?q=repo%3Alinux-test-project%2Flcov+dev%2Fnull+language%3APerl&type=code&l=Perl

Please replace the system specific calls to perl functions where possible - like getwcd() and have the redirection adjusted to File::Spec->devnull();. Both can be seen in the outdated https://github.com/linux-test-project/lcov/pull/155/files.

Also the invocation of perl subscripts should use the current interpreter - that's something done conditional to Win32 in https://github.com/linux-test-project/lcov/pull/74/commits/62f26f8e50066c46b64311ea708a8701e41092be - but I think it is reasonable to always do that.

I don't want to do those a third time... and as it seems to be quite "small" change I hope someone can implement this to the current version removing the need for (too much) patching 2.0 again.

henry2cox commented 1 year ago

Seems moderately straightforward - but I don't have an easily available way to test any changes on windows (can test that it didn't break linux) - so would be better for someone else to tackle. (Might be different if there were internal customers who needed windows...) I do promise to merge any resulting PR very quickly - so nobody has to do this a 4th time :-) Would be great if everything could be tested on MacOS at the same time.

GitMensch commented 1 year ago

Actually: if you could tackle that and make those adjustments "prefer Perl instead of native" and test those on GNU/Linux, then this would already be beneficial (switching to system commands is always an overhead and less portable than using perl).

Background: I have the feeling that native Win32 support will need more adjustments - I'll have a look at this when updating the MSYS2 package (the more that works out of the box then, the better).

henry2cox commented 1 year ago

Most (??) of the changes are in 398fa7ead0ea - but it is possible that I missed some. The result still appears to work correctly on linux - but I don't know what happens on other platforms. I guess: no worse - if those other platforms were broken already.

I don't think this issue can be closed until we know what else needs to be done - and have completed and tested it.

GitMensch commented 1 year ago

That looks promising. I'll report back if/when I'm doing the MSYS2 update to upcoming 2.0.

GitMensch commented 1 year ago

@henry2cox I assume 9113a1f4ecb13408df41e62a5c5103df798d78c7 should take care about some issues? Should I try the MSYS2 packaging with using 2.0 plus https://github.com/linux-test-project/lcov/commit/9113a1f4ecb13408df41e62a5c5103df798d78c7.patch?

henry2cox commented 1 year ago

Yes. That is: I had checked in a bunch of other fixes a few weeks ago - and those are in the 2.0 release tarball. But I had noticed a few others later - and we decided that it was too risky/too potentially destabilizing to jam them into the release. Better to make the release with code we knew works on linux...and worry about windows later. Adding weight to the decision: I'm pretty sure that windows will require more fixes - so there was really no point to add risk when we were pretty sure that we would not have a functional windows version either way.