CI for forward-porting GC3 patches to GC4#147
CI for forward-porting GC3 patches to GC4#147ddeclerck wants to merge 7 commits intoOCamlPro:gitside-masterfrom
Conversation
ddeclerck
force-pushed
the
gc3_to_gc4
branch
3 times, most recently
from
6900d8d to
1c95bd0
Compare
| # check so reporting as portability problem is only noise. | ||
| # This has the effect of redirecting some error messages to stdout. | ||
| # to be moved to the Makefile - currently only usable for bootstrap, | ||
| # but should be done on autogen, too |
GitMensch
left a comment
GitMensch
left a comment
There was a problem hiding this comment.
I suggest to wrap the commits again. From what I've inspected we need one refactor for integrating 4.x logic (you've spotted that well) nicely.
| snprintf (file_open_env, (size_t)COB_FILE_MAX, "%s%s", "IO_", s); | ||
| if ((file_open_io_env = cob_get_env (file_open_env, NULL)) == NULL) { | ||
| snprintf (file_open_env, (size_t)COB_FILE_MAX, "%s%s", "io_", s); | ||
| if (f != NULL) { |
There was a problem hiding this comment.
When/why should f be null here?
There was a problem hiding this comment.
Because some functions (open_cbl_file, cob_sys_delete_file, ...) were "improved" to perform file mapping in GC3 (rev 3944), by calling the cob_chk_file_mapping function, which does not take a cob_file argument in GC3 but does in GC4, and that function in turn calls cob_chk_file_env. Since these functions (open_cbl_file, cob_sys_delete_file, ...) do not use a cob_file object, I resorted to passing NULL and coping with that...
|
oops, hope I haven't broken the gitignore in f36dcda - if not then we likely should apply that to the gcos3x branch as well. |
Saw your message a bit late, added another commit in the meantime 😅 By wrapping up you mean, committing to SVN ? (after doing the requested modifications of course) |
|
This approach is reasonable in general.
... So you did already check that this piece of code changed in a later commit?
If you see a difference in the current code, then you can use "svn annotate" to check which commit did the change in this party of fileio.c to know what to merge before the refactoring.
:-)
Am 1. Juni 2024 00:07:13 MESZ schrieb OCP David Declerck ***@***.***>:
…
@ddeclerck commented on this pull request.
> + /* apply COB_FILE_PATH if set (similar to ACUCOBOL's FILE-PREFIX) */
+ if (file_paths) {
+ for(k=0; file_paths[k] != NULL; k++) {
+ snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s",
+ file_paths[k], SLASH_CHAR, file_open_name);
+ file_open_buff[COB_FILE_MAX] = 0;
+ if (access (file_open_buff, F_OK) == 0) {
+ break;
+ }
+#if defined(WITH_CISAM) || defined(WITH_DISAM) || defined(WITH_VBISAM) || defined(WITH_VISAM)
+ /* ISAM may append '.dat' to file name */
+ snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s.dat",
+ file_paths[k], SLASH_CHAR, file_open_name);
+ file_open_buff[COB_FILE_MAX] = 0;
+ if (access (file_open_buff, F_OK) == 0) {
+ snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s",
+ file_paths[k], SLASH_CHAR, file_open_name);
+ file_open_buff[COB_FILE_MAX] = 0;
+ break;
+ }
+#endif
+ }
+ if (file_paths[k] == NULL) {
+ snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s",
+ file_paths[0], SLASH_CHAR, file_open_name);
+ file_open_buff[COB_FILE_MAX] = 0;
+ }
+ strncpy (file_open_name, file_open_buff, (size_t)COB_FILE_MAX);
+ }
I remember why I haven't refactored that straight away : in case subsequent commits modify the same code, conflicts will be much easier to handle. I was thinking about keeping the refactoring for after all the patches are merged...
--
Reply to this email directly or view it on GitHub:
#147 (comment)
You are receiving this because you commented.
Message ID: ***@***.***>
|
|
I tend to be overly "conservative". Indeed this piece of code is barely modified afterwards, so I'll do the refactoring. |
|
Is this okay to merge (@GitMensch) ? |
|
I'll try to review this (late) evening. |
|
looks_absolute should use "src", not file_open_name directly (merge issue?) "apply_file_paths" should get that via argument as well and have a function comment that it writes to the global buffer. Then add a Changelog "extracted from xyz and also used in abc" to finish that last commit. We either have to remember for later that we need to add a testcase for the new use or (potentially easier) also include it in the last commit as well. |
This change is introduced in a later commit (3993).
Alright ; as for its output, should it write it through its argument or directly to
By "new use", de you mean the fact that we apply file paths to the complex case ? |
yes
good catch - then it is fine to leave as is; if you don't expect any big problem it would be nice to merge that in this bunch to commit that together, but a later bunch is fine as well
Depends on how other functions do it - it is best for now to mimic that (once the merge is completed we may revisit that part, but there are "some" commits left until we get there). |
|
I made the necessary changes.
I find it more convenient to merge consecutive commits. If that's okay for you I could add to the current batch the next eligible commits until 3993 (that would be 6 commits: 3973, 3979, 3988, 3989, 3992 and 3993). |
|
That batch is good to go :-) |
Merged in SVN ;) I see the next commits deal with translation files. Checking the history, it seems those files are usually just copied "as-is" from the GC3 branch to the trunk; is this correct ? |
No, only new files are copied, the others left as-is; before a release I regenerate the files but the files are nearly completely maintained by the translation project. And of course |
|
Alright.
Hope this unfinished sentence did not have any vital info 😅 |
|
I can't remember any important info missing there. |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## gc4 #147 +/- ##
==========================================
- Coverage 66.20% 63.41% -2.79%
==========================================
Files 39 40 +1
Lines 69061 72197 +3136
Branches 19164 20185 +1021
==========================================
+ Hits 45723 45785 +62
- Misses 16147 19270 +3123
+ Partials 7191 7142 -49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Quick question: I sometimes see alternative code for GC4 in I'm talking about those: |
|
That's quite a bunch - any reason to not merge upstream? [we really need to get to commits that have someone else in the ChangeLogs...] |
No good reason. It may be many commits, but the first batch had way more lines (this one is only +1,162 −904).
I'm looking forward to reaching commit 4614 - our first contribution to GC3 ;) |
|
Just a note for a minor issue I've stumbled over when inspecting the last missing "unexpected diff" from this branch: at least run_extensions.at (likely other test sources as well) have which may be adjusted to that will be we may gather all those changes to a single "cleanup testsuite" commit (and may also find uses of |
|
OK, so the only difference for this bunch is that previously "LENGTH OF something" was always pretty-printed without leading zero, while it now is printed as a signed binary. I'm fine with adjusting the expected output in the testsuite for that... And checked with MF: if there is no ODOSLIDE enabled, then the LENGTH OF seems compile-time computed and printed as digit without leading zero or sign; if ODOSLIDE is enabled, then it is printed as a binary (as GC3.3, but with an additional zero instead of the sign) - so I think we really can adjust the testcase's printing here, please do that while I reinspect the bunch. |
|
One note: We should adjust GC3 and merge to GC4 (or possibly the other way around, seems like GC4 has a warning there): MF (with ODOSLIDE) and IBM do not allow the DEPENDING ON item to have a variable position - those have to be before any ODO fields:
|
|
... but the biggest issue is that -fodoslide seems to be not functional with GC3.2 - as the actual values are wrong. (compare to the expected output https://github.com/ddeclerck/gnucobol/blob/7cb4b9eae33e8cd94a983491a0e413a976a86878/tests/testsuite.src/run_extensions.at#L1774-L1813 - MF and GC4 with this PR are identical, apart from how the binary is presented) |
GitMensch
left a comment
GitMensch
left a comment
There was a problem hiding this comment.
apart from fixing the failures by adjusting the expected output of LENGTH OF to be a native binary (=what it currently displays) that bunch seems fine;
as noted, tree.c is currently "strange", merging the fix in the bunch would be good
also: the additional warning for positions of ODO-items should go to 3.x and possibly be an error in both branches (not sure if the current "only GC allows that" is useful); even more important - if 3.x still has the same bad output, that needs to be fixed.
I possibly missed that - what is strange in tree.c (appart from formatting isues) ? |
|
tree.c: just formatting issues |
|
Seems like we need first a CI adjustment 3.x->4.x, then a rebase to get current CI definitions (with increased MSYS2 timeout and ideally IBM LE + most BE) in. |
GitMensch
left a comment
GitMensch
left a comment
There was a problem hiding this comment.
apart from some more GC4 testsuite cleanups (can be done later), that looks good
Yeah I was already doing more adjustments when you wrote this ;) |
|
If that's okay I'll merge that and create a new batch with the newer 3.x commits (5531, 5537, 5540-5542, 5549-5553). |
|
sure, time for upstreaming this batch - again: well done! |
|
About to merge 5541 & 5542 - are you fine with the current formatting of the code in these two commits ? |
|
Just to say: current bunch is good for upstream. You may want to include ibm ci afterwards, then check its state before going on merging and/or inspect the ODO issues in 3.x instead, first. |
Great ; it was just missing the C89/C23 compat fixes (5552, 5553) .
Actually I had merged the IBM CI from 3.X - just forgot to change the target branch... Changed it here so we check how it behaves. |
|
And obviously, we have failures on IBM PPC & Z... |
|
Those failures look older - I'd suggest to get the current bunch upstream (maybe with a new TODO note on PPC + BE), then leaving the IBM stuff for 4.x until you are back. ppc: the align definition stuff seems wrong - needs to check configure and the use of the macros in the headers (possibly something broken on the header split) big-endian: obviously some errors related to that, this is possibly also older (the last time this was verified was around 18 months ago; when Ron left 4.x passed all tests on a similar environment [but that was also before we integrated newer tests]) |
I confirm this is older than the current batch.
Just to let you know, all the PPC failures are due to this __unaligned keyword (a run with this attribute boldy removed works). So appart from this, PPC is fine.
Yup, the S390x errors all seem caused by big-endian - note we also have some in 3.x. |
3 participants
Follow-up of #146.