On 11/14/19 7:26 PM, Chase via cdesktopenv-devel wrote:
> First off, massive apology for the commit size, I did not realize it was
> going to be that big, and there really wasn't a good way to break it
> down that I saw.
>
38MB, yeah... that's pretty much impossible to review properly.
I probably would have done something like:
commit #1 - remove old ksh
#2 - add new ksh
#3 - fix this
#4 - fix that
...
I'm not going to add this immediately to the autotools branch until I
know it builds and actually works. Even then, it might sit in it's own
branch for a bit (autotools-conversion-dtksh, for example).
> A few things to consider before merging:
>
> I am not a lawyer, but it seems that the old ksh version that was
> provided with CDE was donated to the open group by at&t, to be
> distributed as their copyright work under the lgpl, with this patch, the
> copyright will return to at&t, and thus the license control as well (epl
> vs lgpl). It shouldn't be that big of an issue though, we can always
> fork, and I will gladly sacrifice license sovereignty for ~22 years of
> improvements.
>
I don't expect it to be much of an issue, though I am not a lawyer either.
> I tried to make this work with imake, but everything I tried ended up
> with an "error: recipe commences before first target", I did not seem to
> get this error with automake, so into the automake branch it goes.
>
> This patch only works with linux, it needs to be built on the BSDs and
> solarises, shouldnt be that hard though.
>
Does it actually build and work? I noticed that in the configure.ac,
you disabled generation of the relevant Makefiles (though I think the
way you did that is not actually valid WRT use of '#').
Will it actually build and work?
The fact you disabled those Makefiles implies that it does not...
> Somewhere in the dtksh source, I commented out three lines, they
> reference tty_alt, editb and main, these symbols were found in a part of
> the ksh source that didn't seem to get built into the libshell archive,
> but doing so would have alleviated this issue, so I asked the ksh devs
> how I could get these to be added to the archive, to which they
> responded that they have no idea how nmake (the old ksh build system)
> works, only David korn himself knows, and they have switched to meson in
> their builds.
Yes, I am aware of their use of meson. So, would that have to be
installed as well in order for this ksh to build?
Also, it would be nice to know exactly what you changed and where... A
README.md is perfectly fine in programs/dtksh/ ....
> So I believe the best course of action would be to fork
> from here and get automake files into the root to build it (this is why
> I left the meson files in the source).
Yes - we would probably need to do this, or require meson be installed
too. Simply 'forking' would need to be done carefully to allow for
future updates with minimal pain.
The current ksh is undergoing a lot of development - no doubt we would
need/want to upgrade it from time to time.
> Marcin Ciezlak also jumped into
> the dev conversation and said he was looking to make sort of a shared
> library out of ksh (?, I'd have to ask more about it, but I want results
> now, so I am making this commit).
>
Well... It's usually better to do something right than quick... I know
Marcin has talked about that for awhile now, and it would be ideal for
dtksh to simply use an external libksh/libshell library. But yeah, I've
heard that for years now :)
> Sorry again about the commit size, i am going to look for a tool to tell
> me how big my commits will be before I commit them.
>
Size is less of an issue - keeping commits confined to a single purpose
is more important - and use lot's of them...
Even if you had done this with 100 commits, at least they could be
individually reviewed, and problematic ones could be
redone/refactored/fixed.
With this single 38MB commit, it's either take it all or none of it.
I'll need some time to look it over and test... But - only if it will
actually build and work. If not, there is no point.
My first task though is to release CDE 2.3.1 this weekend - a month has
passed since the devel release.
-jon
> Patch is here (my email provider wont send anything over 25M):
> https://gofile.io/?c=yRFsFn
>
> Thank you for your time,
> -Chase
>
>
>
>
> _______________________________________________
> cdesktopenv-devel mailing list
> cdesktopenv-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/cdesktopenv-devel
>
--
Jon Trulson
"Nothing unreal exists."
-- Kiri-kin-tha
_______________________________________________
cdesktopenv-devel mailing list
cdesktopenv-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/cdesktopenv-devel
