vfs/shell: autogenerate shelldef.h, removing code duplication

[tuffnatty]

Proposed changes

Autogenerate src/vfs/shell/shelldef.h.

I see it as the easiest way to prevent bugs like #2347 which are otherwise bound to eventually resurface. E.g., before the change the builtin scripts were trying to use ls -Q GNUism everywhere, breaking the directory listing for non-Linux remotes.

• Resolves: Hardcoded FISH scripts do not work properly #2347

Checklist

• I have referenced the issue(s) resolved by this PR (if any)
• I have signed-off my contribution with git commit --amend -s
• Lint and unit tests pass locally with my changes (make indent && make check)

[zyv]

We had a discussion about this with @aborodin some time ago, where I discovered exactly that and asked whether we should remove this file altogether.

The answer I was given is that this was introduced by Ilia with the justification that for broken mc installs where it can't find the directory with scripts for whatever reason (running from a moved tree with paths hardcoded, incomplete installation, corrupt installation, broken package), FISH is completely unusable.

I think this is, as usual, remarkably bad design: first, the problem with the installation has to be fixed where they occur, and second, if we decide to support this, we have to make it a logical, consistent, and safe system. Unfortunately, as for most other things, I didn't have time to pursue it so far.

If we attack it now, I think we have to ask ourselves, what do we want to achieve here? I think we have two problems:

1. Our built-in scripts are hand-synced, and that's a pain
2. The override strategy is not clearly defined

We can make the following decisions:

1. Re-generate built-in scripts like you did to solve (1)
2. Drop built-in scripts altogether

Regenerating solves the immediate problem and keeps the status quo, so it's a "safe" decision provided that re-generation itself is byte-identical. As in, it correctly handles quotes, etc. - I'd be cautious about that given the POSIX environment differences among targets. Did you test on all weird combinations of troublemakers as in Alpine with dash, Solaris with old sh, OpenIndiana with ksh, FreeBSD with tcsh?

However, if we decide to address it at all, I would like to make a conscious decision about it, because the alternative would have been to remove the built-in scripts completely and simply not bother with the above.

There is a theoretical third alternative - keep the built-ins and remove stand-alone files, but I think this has the disadvantage that the user can't simply copy them to their site directory for customization, so ideally they should stay and your solution (or removing the stand-alone files) is preferable.

I think the "best" solution in terms of the advantages for the user would look like this:

1. We keep the built-in scripts, but regenerate them from stand-alone files like you did here
2. We move the stand-alone files from /libexec/shell to /etc/mc/shell for site customization
3. We keep support for user overrides; they should have priority over site customization

So basically, in addition to what you did, I'd move the files to make it a completely logical design.

Do you agree with my reasoning? Would you be up to doing that? And in any case, thank you for looking into that.

[tuffnatty]

I think this is, as usual, remarkably bad design: first, the problem with the installation has to be fixed where they occur, and second, if we decide to support this, we have to make it a logical, consistent, and safe system. Unfortunately, as for most other things, I didn't have time to pursue it so far.

FISH is bad design as a whole, I would just replace it with SFTP and forget it. But it's a bit more work.

Regenerating solves the immediate problem and keeps the status quo, so it's a "safe" decision provided that re-generation itself is byte-identical. As in, it correctly handles quotes, etc. - I'd be cautious about that given the POSIX environment differences among targets. Did you test on all weird combinations of troublemakers as in Alpine with dash, Solaris with old sh, OpenIndiana with ksh, FreeBSD with tcsh?

It's not byte-identical. First, the scripts were not synced from the start. Then, I generate only what is strictly necessary - e.g. skipping the include-once #defines. I could add them.
As for the quoting, my code handles only ASCII input, it escapes " and \ and adds \n at the end of every line. It does so using $(SED) which usually resolves to gsed. Probably xxd -i or glib-compile-resources or C23's #embed could be used instead, but these solutions have other trade-offs.

It's working in GitHub CI. I believe the GNU Make and the whole autotools system does not support csh at all. I do, however, test my scripts with Heirloom Bourne shell which is close enough to what's in Solaris 8's /bin/sh.

However, if we decide to address it at all, I would like to make a conscious decision about it, because the alternative would have been to remove the built-in scripts completely and simply not bother with the above.

IMHO, when the standalone executable is working, it's better than when it does not.

There is a theoretical third alternative - keep the built-ins and remove stand-alone files, but I think this has the disadvantage that the user can't simply copy them to their site directory for customization, so ideally they should stay and your solution (or removing the stand-alone files) is preferable.

Agreed.

I think the "best" solution in terms of the advantages for the user would look like this:
2. We move the stand-alone files from /libexec/shell to /etc/mc/shell for site customization

What kind of site customization do you have in mind? These are scripts for running on remote systems, not onsite.

3. We keep support for user overrides; they should have priority over site customization

So basically, in addition to what you did, I'd move the files to make it a completely logical design.

Do you agree with my reasoning? Would you be up to doing that? And in any case, thank you for looking into that.

My view is that FISH has some bad protocol decisions which are not easy to fix. I would just replace it with SCP/SFTP/rsync if I had time. Probably it's easier than fixing it.
I am now trying to fix #3128. Not that I had great progress with it, but while doing it I have added some testing infra for fish scripts (#3635).
I do not wish to invest a lot of time in this minor issue. I don't think moving the files to /etc/mc is somehow related to this issue at all.

[zyv]

FISH is bad design as a whole, I would just replace it with SFTP and forget it. But it's a bit more work.

Whether it's bad design or not depends on what one wants to achieve. It was never meant to be a "good" protocol, and you are right that simply replacing it with SFTP would be the right thing to do in an ideal world. It was meant to be a hack that works on all systems with somehow halfway functioning SSH and POSIX environment, but e.g. lacking SFTP. Given this goal, the idea wasn't too bad, but the implementation is certainly not great. But then again, there are too many pieces of mc where the implementation is not great to be polite.

Therefore, I wouldn't want to remove FISH for two reasons: 1) in many cases it's the only thing that kind of works, even now, and 2) our SFTP implementation is absolutely unusable and horrible; see my posts in #3654 for the details.

It's not byte-identical. First, the scripts were not synced from the start. Then, I generate only what is strictly necessary - e.g. skipping the include-once #defines. I could add them. As for the quoting, my code handles only ASCII input, it escapes " and \ and adds \n at the end of every line. It does so using $(SED) which usually resolves to gsed. Probably xxd -i or glib-compile-resources or C23's #embed could be used instead, but these solutions have other trade-offs.

It's working in GitHub CI. I believe the GNU Make and the whole autotools system does not support csh at all. I do, however, test my scripts with Heirloom Bourne shell which is close enough to what's in Solaris 8's /bin/sh.

Alright, thanks for these details. I can't review this in the needed detail anytime soon :( Hopefully, someone else can.

I think the "best" solution in terms of the advantages for the user would look like this:
2. We move the stand-alone files from /libexec/shell to /etc/mc/shell for site customization

What kind of site customization do you have in mind? These are scripts for running on remote systems, not onsite.

Site customization is just standard terminology in distribution and system maintainer speak for changes to the behavior of system packages applying to all users of the system (as opposed to user overrides).

To me, the scripts that are not embedded in mc are configuration, and I see no reason why they should be shipped in libexec other than someone has made a bad decision in the past. By making it "configuration" officially, the admins gain a feature and nothing is lost.

The possible kinds of customizations this would allow to do cleanly are system-wide changes needed for the nodes of the cluster done by the front nodes’ admin, but this, of course, is very theoretical.

I do not wish to invest a lot of time in this minor issue. I don't think moving the files to /etc/mc is somehow related to this issue at all.

I think it is related, and I thought that it's logical and would complete the chain, and is a very minor amount of extra work. But if you are not interested in doing it at all or together with this change, that's fair enough.

[ossilator]

so, the client-side mc embeds the scripts that are deployed to the server, just in case the mc installation is fubar? that's indeed "not reasonable" to put politely. this should be just blown away.

as for site customization, i don't think these scripts are a valid target for that. libexec is exactly where they should live, and no override option is necessary.

[zyv]

so, the client-side mc embeds the scripts that are deployed to the server, just in case the mc installation is fubar? that's indeed "not reasonable" to put politely. this should be just blown away.

as for site customization, i don't think these scripts are a valid target for that. libexec is exactly where they should live, and no override option is necessary.

So, on the valid options matrix, you are for the combination "drop embedded scripts altogether" and "leave non-embedded scripts in /libexec", because you believe that only user-level per-host customization makes sense, correct?

[ossilator]

you are for the combination "drop embedded scripts altogether" and "leave non-embedded scripts in /libexec",

yes

because you believe that only user-level per-host customization makes sense,

no, i don't think that customization makes sense here at all. these programs are integral parts of mc that just happen to be written in sh for practical reasons, not some random setup scripts. you'll find tons of other sh and perl scripts in your /usr/share (hmm, that's actually the correct location, not libexec).
fwiw, kde/kio's fish ioslave ships with a perl re-implementation of the remote stub.

[zyv]

because you believe that only user-level per-host customization makes sense,

no, i don't think that customization makes sense here at all. these programs are integral parts of mc that just happen to be written in sh for practical reasons, not some random setup scripts.

Given the intended use as outlined above (last-resort suboptimally working and poorly performing file transfer system based on SSH), I think it's very reasonable to have per-host user-level overrides for specific scripts that are not working with some embedded whateverbox and are not upstreamable and/or as a stopgap until upstream scripts are fixed that we have now.

you'll find tons of other sh and perl scripts in your /usr/share (hmm, that's actually the correct location, not libexec).

My understanding is that according to FHS, the distinction is that the scripts in /share are examples and additional optional code that is not part of the program and/or not executed by the program, whereas /libexec is for scripts that form part of the program (or are executed by the program) even if they are not directly executable outside the context of the program.

So /libexec is actually more correct than /share, but given the fact that we do have overrides, one can argue that it belongs to /etc, just like the other parts (menu scripts).

fwiw, kde/kio's fish ioslave ships with a perl re-implementation of the remote stub.

I'm aware of it, but I don't think that it's directly reusable for us :(

[tuffnatty]

To me, the scripts that are not embedded in mc are configuration, and I see no reason why they should be shipped in libexec other than someone has made a bad decision in the past. By making it "configuration" officially, the admins gain a feature and nothing is lost.

These scripts (the most complex of them) are very fragile, they're going to break as soon as they are customized, in 90% of cases. I would not expose them more than now, libexec is a fine place, esp. taking into account that they aren't intended to be run on the client side.

The possible kinds of customizations this would allow to do cleanly are system-wide changes needed for the nodes of the cluster done by the front nodes’ admin, but this, of course, is very theoretical.

Hmm... I would refrain from advertising this possibility before fixing #37, #68, #77, #2160, #2256, #2391, #2454, #2561, #3128, #3283, #3635, #3835, #3961, #4279, #4392...

[ossilator]

Given the intended use as outlined above (last-resort suboptimally working and poorly performing file transfer system based on SSH),

i think you are over-interpreting this. it's a highest common denominator kinda thingie, but within these constraints it's expected to "just work".

fwiw, the real baseline is not even ssh, but rsh.

I think it's very reasonable to have per-host user-level overrides for specific scripts that are not working with some embedded whateverbox and are not upstreamable and/or as a stopgap until upstream scripts are fixed that we have now.

these overrides would have to be configurable per target host (not necessarily per user, on either side).
but regardless, i don't think that this would be a realistic way to deal with issues, both because of the fragility that @tuffnatty correctly notes and the sheer effort on the user's side.
also, i think the problem can be ignored at this point. fish is about 30 years old; any relevant compatibility issues can be expected to have been ironed out (todo: sync up with gvfs and kio forks).

My understanding is that according to FHS, [...]

nope. see https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s11.html .

fwiw, kde/kio's fish ioslave ships with a perl re-implementation of the remote stub.

I'm aware of it, but I don't think that it's directly reusable for us :(

it might be that using the perl stub requires also switching to a modified version of the protocol. that's probably a good thing (assuming kde didn't make things even worse), except of course that it would require some porting work.

[tuffnatty]

thanks @ilia-maslakov I've removed shelldef.h from libvfs_shell_la_SOURCES as well.