pipe/fork/exec and friends as custom functions #153
Conversation
…nd symlinking custhelp for uninstalled testing
There was a problem hiding this comment.
Hello @vike2000,
Thank you for raising this issue.
There are some interesting concepts relating to "pipes, forks, and I/O with other processes, etc." in this pull request. We had considered similar concepts for calc v3.
Only built and tested on macOS 13.6.6 using Apple clang 14.0.3, part of Xcode 14.3.1, but my IDE is VSCode, w/ some clangd extension residue .gitignored.
Build req. readline and IDE support py38-pip installed as Macports ports. VSCode clangd support then installed w/ pip-3.8 install compiledb.
We would not want to impose an IDE such as VSCode, add clangd support, nor add things such as pp38-pip, python, and JSON files into calc however.
Before we would be able consider this pull request, we ask that those "extensions" be removed such that this pull request be a pure C addition only.
Please consider revising your pull request accordingly as this will make it easier for us to better evaluate the merits of your overall concept.
I have very little experience in authoring PRs, and not too much experience of C (but some), nor of calc, so please bear with me.
We are happy to be patient as you learn about PRs and explore C as it relates to calc.
There are some tricky concepts involving "pipes, forks, and I/O with other processes, etc.", especially when trying to do them in a portable way that works on a wide variety of systems.
|
Hello :) I said I was unsure whether to add these "development files", so I understand your concerns and have now removed those files. Are my changes to |
|
Hello @vike2000, We might wish to consider adding the general concepts relating to "pipes, forks, and I/O with other processes, etc." to calc version 3. Maybe not in the initial form of this pull request (see our pullrequestreview-2172735939 above), but rather as a new TODO item for calc v3 (see that link for details). The reason why we might do this is because the "pipes, forks, and I/O with other processes, etc." concepts would require a calc version 2.16 release (due to the new builtin functions that are required to be added to the calc language). This is happening when we are trying to get calc stable and ready for a code fork for calc v3. Moreover, such a major change, while very useful, might be better introduced into the newer code base (as pure C). We mention this possibility: not because we dislike the overall concept you are suggesting (indeed we have considered how we might do this in the past) - we do appreciate the overall idea AND we APPRECIATE your interest in calc. We mention this because it might be better to transfer this discussion over in the calc v3 discussion thread. What all that might mean is for us to add some "TODO" bullet point to the top level comment in the calc v3 discussion thread, and to figure out how to best introduce "pipes, forks, and I/O with other processes, etc." into calc. Your ideas seem useful, and you are willing to be patient with us as we are willing to be patient as you explore the world of PRs and calc code, then exploring this in calc v3 world might produce the best outcome. |
Adding things to |
|
BTW @vike2000, In regards to the defer to calc v3 comment we made above. That does not represent a final decision, but rather our "thinking out load". We will still ponder your pull request. |
I'm sure there are. Just recently I saw I had an |
|
Hello @vike2000,
Over the lifetime of calc, a number of things have started off as calc scripts and custom functions. Often as you start to use some initial implementation of some new concept, one finds one needs additional functionality, additional parameters, and adjustments to the original concept. Gaining experience with the new concept before the implementation becomes "baked into the main code base" (as the express goes) is a very good idea as once it enters to main calc code base, it becomes harder to improve and expand it (as some people start using the original concept as is and become dependent on behavior that later experience suggests needs to change). One path ("thinking out load" again) is for you to gain experience with concepts relating to "pipes, forks, and I/O with other processes, etc." and bring that experience to the calc v3 discussion thread. Again, we appreciate your willingness to contribute to calc, to learn about PRs and to learn about calc internals. We mention this because we don't want you to become discouraged just because this PR readiest has not been immediately added to the calc code base. Overall the concepts relating to "pipes, forks, and I/O with other processes, etc." are worth doing well and that can take time. |
No rush on my part. A |
There was a problem hiding this comment.
Hello @pmetzger,
As you may see, @vike2000 has revised their pull request and has focused on the core concepts relating to "pipes, forks, and I/O with other processes, etc.".
Please re-consider reevaluating this PR, keeping in mind that this is @vike2000's first calc PR and that @vike2000 is learning about both PRs and calc internals. That @vike2000 is willing to do that gives us some confidence.
Sure, there may be ways to improve on the implementation: but that is the way development goes sometimes. :-)
Also consider weighing in, if you please, on if the core concepts relating to "pipes, forks, and I/O with other processes, etc." are best defended to calc v3 or introduced as custom functions in a calc 2.16. We would value your consideration on those possibilities.
|
I'm finding reading large chunks of this very difficult to read because of unusual choices in code formatting. It would be best if these were cleaned up and made more conventional. Example: 
|
Fair points, @pmetzger. Perhaps @vike2000 you could adjust? BTW: Even if we end up deferring this PR to calc v3, getting this PR into existing shape might be worth the effort so that we may reference it as concept for a calc v3 TODO. |
|
@vike2000 There are a lot of other places with similar issues. I'd go through all the submitted code if I were you and try to make it as readable as possible. |
… and commented out code
I can understand that. I've pushed two fixes of the worst formatting.
May I ask if you could point me to some examples of places you want fixed in the current set of files? I also want to thank you for your time and willingness to help me try to get this PR to a good enough level this far - thank you both! |
|
Just a heads up, @vike2000: Calc v2 is moving towards maintenance modeThe code base from which calc v3 will be formed is being finalized, and so a major new feature won't be available for calc v2 as that version is going to go into stable maintenance mode (fixing only critical bugs - no new features). We released calc v2.15.1.0 is preparation for the above. (There may be a v2.15.1.1 before the calc v3 fork 🍴). Anyway that code base had to undergo an (some say unfortunate) change of white space (TAB removal from source: see issue #103 from that TAB discussion). As a result, your pull request you submitted has to be adjusted as the files underneath it changed. Sorry (tm Canada 🇨🇦) to ask you to change your PR again. Think of this as good pull request practice 🤓. Calc v3 and your ideasWe have added a TODO in the top comment of issue #103. New projects often start out as evolving conversations as the TODO list in the opening comment gets edited and revised over time. Your concept was added to the bottom of the TODO (not out of priority, but because it was the most recent - later on as the project evolves certain TODO items will spin off into their own "enhancement project" issues and the TODO bullet point will become a "finish issue XYZZY" - for an example of arch a evolved "enhancement project" see issue 2239 of the temp-test-ioccc repo ). You may notice slightly above this comment that GitHub added a notice that this pull request was referenced elsewhere. See the previous paragraph for details as to what and why. How this pull request will be usedBack to this pull request. We highly recommend that you work with @pmetzger to get this pull request compatible with the evolved code format, and to address his suggestions about formatting. Even though we will eventually reject this pull request, the reference from the link from the top comment of issue #103 to this (eventually closed PR) will remain. It would be a good thing (and good practice) to get this pull request into good shape for the later calc v3 code. Working towards an new calc v3 enhancement projectIn time (and after the calc v3 code fork) as work on the TODO list of issue #103 starts, a new Enhancement issue will be created with a title along the lines of this pull request. The top comment of that new calc v3 issue will include a link to this pull request as a starting position. Comments in that "new enhancement issue" will include ideas about the topic. BTW: So far we have held off on making architecture comments and portability suggestions: that "new enhancement issue" is the proper place for those suggestions. This is one reason why it would very welcome for this pull request to be polished and merge ready: this serve as a starting place for "new enhancement issue" for calc v3 project. A bit of calc history and legacyCalc has a multi-decade timeline with lots of users from various platforms and resources (some hardware platforms, some supercomputers, some cell phone apps, some mathematical, some cryptography, some space probes, etc.). And while backward compatibility is critical to those many users and developers and platforms, it makes it hard to make the sort of improvements we want to make to make calc better. Calc v3 will, for the first time in about 25 years, change the memory layout of its fundamental numeric objects because limiting numbers to 4 GB is now a problem for some tasks. (We know on one ongoing calculation that has been running for years: freezing calculation checkpoints from memory to disk and restoring memory into faster hardware. We plan a memory migration strategy for them from their calc v2 memory layout to the new calc v3 memory layout, for example). So calc v3 was formed to make such changes while calc v2 will remain frozen 🥶 in maintenance mode. Topics such as this "new enhancement issue" will be ready for calc v3. Once the fork for calc v3 occurs, projects like the "new enhancement issue" your pull request inspired will spring up. Meanwhile please 🙏 pardon our project planning dust as we get ready for the fork 🍴. Sorry the the lengthSorry (tm Canada 🇨🇦) for the length and complexity of this comment. We wanted you, @vike2000, better understand what is happening and why. |
|
Ah, I see, and understand my PR inadvertently came at a time of great changes :) No worries - I'll bide my time, try to keep track of the progress of the project, and jump in when I feel the calling. I am away from home this week and will have little time and bad access to my tools, but when I'm back I'll swiftly do a tab to space conversion. I'm also thinking I'll look at running clang-format or similar (or even do some massive tweaking of a gawk-script i've dabbled with for some miscellaneous color-coding and block balancing over the years).
I'm curiously awaiting following along in such improvement discussions :) Thank you for getting me up to date, and history no less - I must say I hadn't realized |
Fix system include file.
Try to fix more compile problems under Linux.
We undo the attempt to fix the code by the previous commit.
There was a problem hiding this comment.
ACTION REQUESTED
We cannot commit this PR as is, @vike2000. While it might compile under macOS, it does not compile under Linux such as the current Red Hat RHEL9.5 or say AlmaLinux release 9.5 (Teal Serval) or ubuntu-24.04 as used by CodeQL.
The function reallocf(3) is not widely supported, please rewrite to use realloc(3).
The macro FD_COPY(3) is not widely supported. Please adjust the code to not depend on that macro.
The function basename_r(3) is not widely supported, please rewrite to use basename(3).
The function dirname_r(3) is not widely supported, please rewrite to use dirname(3).
UPDATE 0a
We did make an online attempt to do the above with commit 6a80a247407768b984a7724b11ef6e4cbce169bc, but our attempt was invalid/incorrect so we reverted that attempt with commit 84f74d95877b26a8b328a5824f03b4b67c62625e.
We did make some changes to account for the recently updated code in the master branch such as with commit b429fd8e194f467be2a0594edeb268455ddbee16 and with commit b7bd76b825e430e6168bd515b36d6bed778fa03c and then commit f33aa19f97adb534c902020bd25eff25f49b6153, however we need you to do the rest as noted above.
See the Autobuild errors in the CodeQL run, @vike2000.
Once you address those errors so that the PR may be added without warnings or errors, then we will process this PR, consider how to integrate such custom functions into builtin functions, improve the help file (by creating a set of individual custom help files), and to perform lots of testing (including adding tests to cal/regress.cal). However before we consider that, we would like your PR to pass the CodeQL run under Ubuntu (CodeQL uses ubuntu-24.04, FYI).
Or you could cancel this PR, build a new PR based on the current top of the master branch using the requested changes as noted near the top of this comment, and re-submit a new PR. Your choice: modify this PR or build a new PR.
Thanks!
|
Sorry for the long delay. I got busy with other projects and might've thought these changes would require more work on my end but I hope there's not much more to the critical fixes than what I've done in my one commit now. I'm not sure why I got two merge commits as I tried to pull the latest before pushing. I did get a conflict in I have still only tested it on macOS, hoping it will work in the CodeQL, and only if we can't see a clear fix I'll try to do more work on an Ubuntu in Docker on my wobbly system here... Hope I haven't overlooked anything and looking forward to see how this progresses :) |
We will look at your mods in terms of making them custom functions just prior to the calc v2 / calc v3 fork. As custom functions we don't need to be as concerned about interoperability with existing implementations. Moreover when calc v3 goes forward, we can look at bringing in the pipe/fork/exec and friends in to the main stream while retiring the custom functions in calc v3 but leaving them in calc v2 as custom functions. BTW: Once the calc v3 fork happens, it will be a while before calc v3 is "production ready". So leaving in pipe/fork/exec and friends in calc v2 as custom functions seems fine. We are busy with getting ready for IOCCC29, so it may be "a bit" before we review your PR, so please be patient. We don't plan on modifying calc much except for a what is needed to fix critical bug related to heap-use-after-free memory errors, which shouldn't impact your PR. Thanks again for updating your PR, @vike2000 👍 P.S. We like the "pipe/fork/exec and friends" name for your concept. Very friendly. 😉 |
There was a problem hiding this comment.
We are curious about these additions to .gitignore (not that we have any problem when them), @vike2000:
Can you tell us about each of the ignored additions? Some seem clang related and have hints at being useful. What do each of the excludes do?
Ignoring custom/.gitignore is an interning choice. We might skip that addition when we "mod your mod" to the .gitignore file.
How are you using such files?
Are you willing to give us, where makes sense, what sort of content they contain>
Perhaps, where reasonable and you feel comfortable, you could also attach ignored files as a tarball.
This won't impact our processing your PR: we just are interested in knowing.
lcn2
dismissed
their stale review
The requested changes appear to have been addressed. And while we will review the updated PR, we can now sadly dismiss this previous review.
There was a problem hiding this comment.
Our plan is to make your PR the basis for calc version 2.16.0.1, and so we will "mod your mod" and update CHANGES accordingly, including moving your "Adding support for pipe/fork/exec" change into that new section.
We will add a TODO about the pending changes prior to the calc v2 / v3 fork.
custhelp
Outdated
There was a problem hiding this comment.
What is the purpose of this top level symlink, @vike2000 ? Is it needed or was it left in by mistake?
The custom help files will work once calc is installed.
BTW: We had to chmod 0444 custom/pfe
In the mean time, if you want to test something like help pfe before installing, use:
make run
This will launch the ./calc with the proper environment variables so that things like CALCCUSTOMHELP point to your local directory tree.
There was a problem hiding this comment.
👍 although we will update as per make depend by a "mod of your mod".
There was a problem hiding this comment.
OK, good catch. 👍
We wonder if this mistake you corrected has happened elsewhere in calc.
There was a problem hiding this comment.
👍 (except for a minor bit of trailing whitespace where we will "mod your mod" 😉)
There was a problem hiding this comment.
Thank you for the help file! Too many contributors fail to add this!
We plan to address the trailing whitespace and convert to the ASCII tab into spaces as your terminal appears to use a 4 character tab stop that others might not use. Better to use just ASCII space as they are more "portable" 🤓. So we plan to "mod your mod" 😉.
There was a problem hiding this comment.
Thanks for the custom/pfe.cal file!
A similar note about ASCII tabs, including ASCII spaces before ASCII tabs, applies.
BTW: W hope you don't take offense to our comments. They are just part of our review.
We DO APPRECIATE the effort you have put into this PR and hope you will take these comments in good form.
There was a problem hiding this comment.
At first glance looks OK. There are a few minor whitespace issues and some line lengths that picky(1) would object to that we plan to address in our "mod to your mod". Otherwise generally a good job. Thanks @vike2000 👍
We will do some minor formatting to fit calc v2 might a weeeee bit. Also in calc v2 we sadly cannot assume
that // is a valid comment as some compiler environments where calc v2 is used do not understand that syntax. This will change in calc v3, but for now we will have to modify /* such comments */ .. sigh .. not at you, at them.
Some older C compilers do not like if things are declared in the middle of a function. So in calc v2 we try to avoid that. This will change in calc v3, but for now we will have to modify stuff according. Also apparently initializing a variable with a constant is OK, but assigning it an expression is not. sigh .. not at you, at them.
in general we apologize for some "formatting edits" .. we did this because of feedback from a old environment that we will no longer support in calc v3.
How odd .. some calc v2 supported environment throws a fit because the same 3rd argument parameter to select(2) is used as the 4th parameter. Easy to fix .. sigh .. not at you, at them.
It looks that there are some warnings regarding strlcat(3) that we have to address due to another system's report. Sadly strlcat(3) is not portable, so we will have to include the source to that function. For easy we might just add it in as a static function to custom/u_pfe.c. That is a nice function, but for calc v2 land, is it not universal. sigh .. not at you, at them.
And one broken code analyzer complains about what is effetely formatting. Even though there are other places with the calc source code that do the same thing, the custom/u_pfe.c isn't in their exception list so we have to do a work-a-round. This is doable .. just a pain. sigh .. not at you, at them.
Someone had some custom code when a function that conflicted with custom/u_pfe.c function. Not one of the exported ones for "pipe/fork/exec and friends", but one of the internal functions. Understandable as all non-static function symbols are exported to the custom library. The easy workaround is to make anything not exported from custom/u_pfe.c a static function. This is doable .. just a pain. sigh .. not at you, at them.
It turns out that in the process of making non-exported functions static that another environment complained about functions being defined but not used. We will #if 0 ... #endif out that code.
We needed to perform a make depend to update custom/Makefile.
In calc v3, a bunch of this stuff won't matter because we intend to change the memory model (to allow for values larger than 2GB in size) and those systems will be left behind with calc v2. However before we get to calc v2, we need to pay attention to such systems. sigh .. not at you, at them.
We are not complaining, just explaining what we are doing.
There was a problem hiding this comment.
Thank you @vike2000
With a few "mods to your mod" 😉 were we will attempt to update your pull request, we plan to go though the process of building a calc v2.16.0.1 based on your code.
We will stat that release process a bit later (time between meeting do do this): we need to start our next meeting now and we have learned the hardware that the release process should NOT be rushed.
As mentioned in various comments in PR lcn2#153: lcn2#153 we needed to modify the PR you sent us for various reasons, including but not limited to having to satiny a number of older systems on which calc v2 needs to run. Please examine these "mods to your mods" and see if they are OK form your perspective. Feel free to update your PR if you find any issues. Once you approve or update your PR, we will complete our review.
|
OK, we have "mods your mods" ready. Our commit of "mods your mods" is: We did a Please update your Let is know if that is OK, of if need, update your PR. We will await your review. Questions and command welcome. UPDATE 0bOops, we found an issue if our "mods to your mods" on systems that have We were also reminded that, for calc v2, we have to use See commit 3112846 for a mod or our "mods to your mods" :-). |
On systems that have `strlcat(3)` and `strlcpy(3)`, not everyone uses the same function prototype. We renamed the static functions in `custom/u_pfe.c` to begin with `private_`.
For calc v2, we need to declare static functions with `S_FUNC`.
|
Please see our edits to this PR, @vike2000 Let us know if all is well on your side and we will process this PR. If needed, modify this PR and we will evaluate. Thanks 👍 |
Greetings!
Wanting to support external shell/binary cli use in
calc; sincesystem()doesn't catch and return output, as in clicalc '2+system("echo 3")'.Hopefully the
custom/pfehelp-file synopsis outlines the added functions. If you think I should reproduce or reference more of the underlying syscalls' docs, I'll try.I have very little experience in authoring PRs, and not too much experience of C (but some), nor of calc, so please bear with me.
I don't know how much of 3f106bf you want or whether 1d70a1f and 70d93c2 should be in separate PR(s) so please tell me what you think about that, if and how I should modify or recreate this PR.
Only built and tested on macOS 13.6.6 using Apple
clang14.0.3, part of Xcode 14.3.1, but my IDE is VSCode, w/ someclangdextension residue.gitignored.Build req.
readlineand IDE supportpy38-pipinstalled as Macportsports. VSCodeclangdsupport then installed w/pip-3.8 install compiledb.I've tried to adhere to perceived convention/style in
.h/.c, albeit loosely vis-à-vis heavy tabbing for column alignment. In.caland.shI've allowed myself my own style - sorry; of course I'll reformat if requested.As with everything else, naming and/or prefixing is open for discussion, especially in
pfe,pread, andpwrite, being higher level utilities implemented first incalc/.caland then in.h/.c.I am a bit unsure whether I am responsible for any more
free()than I do. I tried doingleaks(1) at some point but didn't know how to investigate from there.Some of the functions use "addr" features (where support functions are named
…ref…), some use optional arguments (valv_opt…) and type checking in combination, and some use all in combination.Only a minuscule subset of possible usage alternatives there are tested.
We could also do more sanity checks after optional/type checking.
If these functions should try to be compatible with general file-handles like from
fopenorcalcs equivalents, I'd have to investigate that.Grateful for any suggestions!