Ticket #3642 (closed defect: fixed)
--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
Change History
comment:1 Changed 9 years ago by zaytsev-work
- Priority changed from major to minor
- Milestone changed from Future Releases to 4.8.18
comment:2 Changed 9 years 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 9 years 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 9 years 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 9 years ago by gv
It was not lost. It worked on _every_ version on mc I remember. And why throw-it away?
comment:6 Changed 9 years 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 9 years 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.
comment:8 Changed 9 years 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 9 years 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 9 years ago by gv
Yep. Now it's working again. Thx.
Fairies... :-)
comment:11 follow-up: ↓ 12 Changed 9 years 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.
Changed 9 years ago by andrew_b
- Attachment 0001-Ticket-3642-make-with-subshell-optional-working-agai.patch added
comment:12 in reply to: ↑ 11 Changed 9 years 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 9 years ago by andrew_b
So, will we fix this (and another) bug(s) without tests?
comment:14 Changed 9 years 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 9 years 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 9 years 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 8 years ago by andrew_b
- Branch state changed from no branch to on review
Branch: 3642_subshell_optional
changeset:22c3a35e8cc5d1adc50dd1df580e550cd3bfd8a5
comment:18 Changed 8 years 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 8 years 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
Merged to master: [1f5e93efc9ebc824b8845b67e8b8b24e60d4e7ad].
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...