CI for forward-porting GC3 patches to GC4

[ddeclerck]

trafficstars

Follow-up of #146.

[GitMensch]

oops, hope I haven't broken the gitignore in f36dcda83d0bbfd6b03410e566680384b54dae43 - if not then we likely should apply that to the gcos3x branch as well.

[ddeclerck]

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.

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)

[GitMensch]

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: https://github.com/OCamlPro/gnucobol/pull/147#discussion_r1622987080 You are receiving this because you commented.

Message ID: @.***>

[ddeclerck]

I tend to be overly "conservative". Indeed this piece of code is barely modified afterwards, so I'll do the refactoring.

[ddeclerck]

Is this okay to merge (@GitMensch) ?

[GitMensch]

I'll try to review this (late) evening.

[GitMensch]

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.

[ddeclerck]

looks_absolute should use "src", not file_open_name directly (merge issue?)

This change is introduced in a later commit (3993).

"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.

Alright ; as for its output, should it write it through its argument or directly to file_open_name ?

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.

By "new use", de you mean the fact that we apply file paths to the complex case ?

[GitMensch]

By "new use", de you mean the fact that we apply file paths to the complex case ?

yes

This change is introduced in a later commit (3993).

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

Alright ; as for its output, should it write it through its argument or directly to file_open_name ?

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).

[ddeclerck]

I made the necessary changes.

This change is introduced in a later commit (3993).

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

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).

[GitMensch]

That batch is good to go :-)

[ddeclerck]

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 ?

[GitMensch]

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. So just record the merge, then copy over new files and svn add them, if there are any.

And of course

[ddeclerck]

Alright.

And of course

Hope this unfinished sentence did not have any vital info 😅

[GitMensch]

I can't remember any important info missing there.

[codecov-commenter]

:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

:x: Patch coverage is 91.85668% with 25 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 63.41%. Comparing base (14e2140) to head (7eea649). :warning: Report is 20 commits behind head on gc4.

Files with missing lines	Patch %	Lines
libcob/move.c	90.04%	9 Missing and 12 partials :warning:
cobc/tree.c	94.91%	0 Missing and 3 partials :warning:
libcob/termio.c	90.00%	0 Missing and 1 partial :warning:
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ddeclerck]

Quick question: I sometimes see alternative code for GC4 in #if 0 blocks; I guess I should implement those and drop the other branch, correct ?

I'm talking about those:

#if 0 /* TODO for 4.0: set the attributes from the field given outside on the stack */
		output ("cob_field *cob_fret, const int cob_pam");
#else
		output ("cob_field **cob_fret, const int cob_pam");
#endif

[GitMensch]

That's quite a bunch - any reason to not merge upstream? Open issues you are aware of or special adjustments needed?

[we really need to get to commits that have someone else in the ChangeLogs...]

[ddeclerck]

That's quite a bunch - any reason to not merge upstream? Open issues you are aware of or special adjustments needed?

No good reason. It may be many commits, but the first batch had way more lines (this one is only +1,162 −904). Anyways, I'll merge upstream if that's okay with you.

[we really need to get to commits that have someone else in the ChangeLogs...]

I'm looking forward to reaching commit 4614 - our first contribution to GC3 ;)

[ddeclerck]

Ready to merge this batch - just awaiting confirmation ;)

[GitMensch]

confirmed

[ddeclerck]

In the process of merging commit 4332 from GC3 to GC4. It introduces many new exception definitions in exception.def, in particular COB_EC_CONTINUE (1800). However a (non-standard ?) exception EC-DELETE-FILE with code 1800 was also introduced in GC4 by commit 3645.

Are those numbers somehow standardized ? Can I just give a different code to EC-DELETE-FILE ?

[GitMensch]

The numbers are only internal, feel free to adjust new ones during merge as you see fit. For the delete file exception - this was part of a working draft for the standard and was later changed - if you could try to fix that as a new commit targetting 3.x (and may be merged together with these the referenced merge commit or in a later one) that would be very good.

Working draft: there is this exception to be able to recognize (I think the case of the file not being there and "success" NOT ON EXCEPTION be the result).

Standard (that's much better):

• the matching IO status is set; so for example 37/39 for files with missing access rights, different file attributes; 05 for "file was not there"
• the EC-I-O exception matching the io status is set

I think I've missed that later change in the standard and therefore had not adjusted 3.x. Can you please check its current state here? Could you please send a PR to fix what's needed to be fixed?

[GitMensch]

I suggest getting the commit of #152 also into gc4 branch if it isn't in yet (therefore it will also be used for the PR checks), then get this current bunch into upstream again.... but don't feel any pressure for each of those, you can add more merge-commits as you see fit and also skip the CI adjustment.

[ddeclerck]

I suggest getting the commit of #152 also into gc4 branch if it isn't in yet (therefore it will also be used for the PR checks), then get this current bunch into upstream again.... but don't feel any pressure for each of those, you can add more merge-commits as you see fit and also skip the CI adjustment.

Actually, the contents of #152 is already present both in the gcos4gnucobol-3.x and gc4 branches.

Indeed it's time to merge upstream again, as the next commit might be a bit tricky (EXTFH changes, though it's a merge commit from trunk, so it may in fact just be trivial).

[ddeclerck]

I think that is enough for this batch ;).

[GitMensch]

That batch looks good for me as well. I guess the EXTFH merge commit was not included yet - or was it just trivial?

[ddeclerck]

That batch looks good for me as well. I guess the EXTFH merge commit was not included yet - or was it just trivial?

Well, it was trivial to merge, as it was a merge commit from trunk to gnucobol-3.x. Though it did take some time to check that everything from this commit was "still there" in trunk, considering that things changed in trunk in the meantime.

[ddeclerck]

And here is another batch. Not too big this time. Next batch will (finally) include some of our contributions ;)

[ddeclerck]

Well, I guess that's it for this (quite big) batch. Is that okay for you @GitMensch ?