Ticket #3642 (closed defect: fixed)

Opened 19 months ago

Last modified 17 months ago

--with-subshell=optional does not work

Reported by: gv Owned by: andrew_b
Priority: minor Milestone: 4.8.18
Component: mc-core Version: 4.8.17
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

Starting with mc 4.8.17, the --with-subshell=optional argument passed to configure keep mc subshell enabled.

Attachments

0001-Ticket-3642-make-with-subshell-optional-working-agai.patch (851 bytes) - added by andrew_b 19 months ago.

Change History

comment:1 Changed 19 months ago by zaytsev-work

  • Priority changed from major to minor
  • Milestone changed from Future Releases to 4.8.18

Are you sure it started with 4.8.17 ? Grepping for SUBSHELL_OPTIONAL, I can't see it used anywhere, so no wonder it doesn't work...

comment:2 Changed 19 months ago by gv

Yes, 100%. I build every version on several distributions of Linux. None works now. All worked up to 4.8.16.

comment:3 Changed 19 months ago by gv

Adding "#define SUBSHELL_OPTIONAL 1" in config.h make no difference.
The strange thing, there is no SUBSHELL_OPTIONAL in 4.8.16's config.h. But somehow. it just work.

comment:4 Changed 19 months ago by zaytsev-work

Sounds like it's got lost between 4.7.5.6 and 4.8.0 and nobody has noticed it in the last 5 years. I think that we should just throw this option away, or else, re-add it, but only with a test to ensure that it doesn't get lost again, otherwise it's wasted time and effort.

comment:5 Changed 19 months ago by gv

It was not lost. It worked on _every_ version on mc I remember. And why throw-it away?

comment:6 Changed 19 months ago by zaytsev-work

I can't build test it right now, but from cursory grepping I see no possibility how it could have worked in 4.8.16, because the code in the tarball doesn't contain any relevant references to SUBSHELL_OPTIONAL. I don't believe in magic subshell disabling fairies, you know. If you can bisect it to show which commit exactly between 4.8.16 and 4.8.17 broke it, this will be much appreciated.

And why throw-it away?

If I am right that it was lost 5 years ago, and nobody has complained so far, then it doesn't sound like something that users can't live without. And if there is no test for it, it can vanish anytime without anyone noticing until it's too late.

comment:7 Changed 19 months ago by gv

Replying to zaytsev-work:

I can't build test it right now, but from cursory grepping I see no possibility how it could have worked in 4.8.16, because the code in the tarball doesn't contain any relevant references to SUBSHELL_OPTIONAL. I don't believe in magic subshell disabling fairies, you know.

OK, try-it. Your gonna be amazed. Fairies or not.
For me, every version worked fine on AIX and Linux (cento 6 and 7, fedora, openwrt).

If you can bisect it to show which commit exactly between 4.8.16 and 4.8.17 broke it, this will be much appreciated.

Ok. I will try on the next weekend.

If I am right that it was lost 5 years ago, and nobody has complained so far, then it doesn't sound like something that users can't live without.

No one complained because it just worked.

And if there is no test for it, it can vanish anytime without anyone noticing until it's too late.

Agreed.

Last edited 19 months ago by gv (previous) (diff)

comment:8 Changed 19 months ago by gv

The problem is somewhere in lib/shell.c
Using shell.c from 4.8.16 in 4.8.17 make the the shell optional again. The person who split mc_shell_recognize_and_fill_type() function did not do a very good job.

comment:9 Changed 19 months ago by zaytsev-work

Yay, I can now see why I couldn't find any references to SUBSHELL_OPTIONAL, it's because I had lib on my "Enable ignore directories" list! :-(

If what you're saying is correct, then the bad commit must be this attempt to fix a segfault: changeset:83b02196, and specifically this is most probably wrong:

mc_global.tty.use_subshell = mc_shell->type != SHELL_NONE;

and should be something like

mc_global.tty.use_subshell &= mc_shell->type != SHELL_NONE;

comment:10 Changed 19 months ago by gv

Yep. Now it's working again. Thx.
Fairies... :-)

comment:11 follow-up: ↓ 12 Changed 19 months ago by zaytsev-work

Okay, now the question is how do we make a test for it? I was not the one who broke it, but just fixing it without a test is road to nowhere. This whole subshell crap is so fragile that next time there is any change to add a new feature or to fix a bug, this will get broken again.

Fairies... :-)

Yeah, it seems that the ignore directories thing can easily lead to much embarrassment :-( Imagine my reaction when I search the source code for the use of the flag, and it's simply not used in both allegedly working and non-working versions... Anyways, it turned to be a red herring either way, because the flag got reset much later in a botched attempt to fix a segfault. I hate software.

comment:12 in reply to: ↑ 11 Changed 19 months ago by gv

Replying to zaytsev-work:

Okay, now the question is how do we make a test for it?

I cannot help here. I'm not aware with the (automatic) test system for mc (if it is one).
All I can say that I will continue to use --with-subshell=optional and if it's not going to work again I will fill another bug report.

I was not the one who broke it

Does not matter who did-it. We all make mistakes from time to time. But we find them and we correct them.

Yeah, it seems that the ignore directories thing can easily lead to much embarrassment :-(

Relax. :-) It can happen to anyone.

I hate software.

Nah. You're just upset. :-)

comment:13 Changed 19 months ago by andrew_b

So, will we fix this (and another) bug(s) without tests?

comment:14 Changed 19 months ago by zaytsev

So, will we fix this (and another) bug(s) without tests?

I don't know, what do you think? I find that it's very disappointing that almost every time we try to fix something, a new annoying problem is introduced. Of course, this doesn't mean that we shouldn't keep trying fixing problems. However, I think it's definitively an indicator that we should really start seriously worrying about writing more tests.

We have some simple check unit tests, of which one could make much more use. On top of that, I'm thinking of doing some basic smoke / integration tests with the built-in autotools thing, if Andreas will not beat me to it, e.g. run all of our Perl stuff through perl -w at least to confirm that the syntax is correct.

Of course, it's hard to test C stuff, we have tons of code that isn't covered in any way, and it's not realistic to cover all of it, but I really think we should start doing something about it, and one way to go about it is to try to add a regression test for bugs if it's practical.

Now, the biggest problem that I have with reviewing branches so far, is that it requires an insane amount of effort to do it properly, because there aren't any tests at all that come with it. It's unclear if the stated behavior is really implemented and the only ways to know is to try to do some manual testing, and mentally trace the possible branches by playing compiler in my head. However, I'm not a machine, and I make mistakes and miss errors, which subsequently leads to frustration.

If we would require tests for any non-trivial changes and it was practical to develop them, we would be able to process the contributions waaay faster than we are currently able to.

comment:15 Changed 19 months ago by zaytsev

An additional consideration that I have is that now that the release process has been more or less ironed out (for me), I would suggest to start doing the following:

  • we merge everything that we think should go into release, including cleanups
  • we announce the intention to make a release on the list and cut a tarball from master
  • we let whomever is willing to take the time to test the tarball for two weeks
  • in the mean time, we work in branches, and only commit translations, etc. to master
  • if regressions are found we try to fix them on master
  • we aim to make a full official release within two weeks

Of course, this will not solve all problems due to lack of automated tests, but at least it will hopefully help to avoid stupid FTBFS, etc.

comment:16 Changed 19 months ago by zaytsev

  • Status changed from new to accepted
  • Owner set to zaytsev

So, will we fix this (and another) bug(s) without tests?

Regarding this bug, could you please keep it open until I'm back? It doesn't look critical at all, and now that we found what was the problem, it's trivial to fix.

However, I would like to try to write a regression test for it. I have the following idea: the global structure can be initialized with use_subshell set to FALSE, then I can let all initialization magic run, and check that use_subshell is still FALSE. If you can && / || want to help me with that, I will be very happy. If not, then I would like at least to give it a try... If I will fail, we can merge it as is at any time later.

comment:17 Changed 17 months ago by andrew_b

  • Branch state changed from no branch to on review

comment:18 Changed 17 months ago by andrew_b

  • Owner changed from zaytsev to andrew_b
  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:19 Changed 17 months ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

comment:20 Changed 17 months ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.