Open lefessan opened 11 months ago
Attention: Patch coverage is 85.46512%
with 25 lines
in your changes are missing coverage. Please review.
Project coverage is 65.90%. Comparing base (
89c45a3
) to head (75646da
).
Files | Patch % | Lines |
---|---|---|
cobc/cobc.c | 86.46% | 6 Missing and 12 partials :warning: |
cobc/error.c | 76.47% | 2 Missing and 2 partials :warning: |
cobc/pplex.l | 86.66% | 1 Missing and 1 partial :warning: |
cobc/typeck.c | 0.00% | 0 Missing and 1 partial :warning: |
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
As testcases are missing I cannot get a grasp what this is about....
We definitely should provide a way to "only" calculate the dependencies and in this case the paths are not available so we can only output the names, which may be the idea behind that.
Not sure why we should have a "only on one line".
Adjustments to -MT and friends are useful and those were added back with an explicit note that there will be changes - but those should mimic what GCC does with them so please provide sample code and output of a C program with #include <something.h>
and #include <else/something.h>
and #include "other/something.h"
along with some different -I and -M... options and add test cases that have the same scenario for cobc - making the later mimic the first is the way to go. Only when we have that we should inspect your scenario to check if that's what you need or what is missing.
And yes: if you could tackle the -M... options, that would be quite good (but those changes should mimic recent GCC, both from their options and from their results).
I added a test in used_binaries.at
... and most arguments used by gcc
that make sense for GnuCOBOL. I don't know which changes are being done in gcc
, but I can import them accordingly.
For -fcopybook-deps
, the idea is to output dependencies for other tools than make
, in a very simple format and without file path resolution. For example, using this output, it is possible to create a tool that will use it to know the copybooks of a source file, and then search for them by scanning directories, and finally call cobc
again with an automatically generated list of -I <dir>
.
@GitMensch I have a completely unrelated question ; I have a PR on the manual in https://github.com/OCamlPro/gnucobol-docs/pull/2 . Is there another maintainer for that part, or are you also in charge ?
Should
-MM
and-MMD
be added, ignoring the copybooks inCOB_COPY_DIR
(currently extfh and screenio)?Shouldn't the dependency options be included in gnucobol.texi, possibly copying GCCs docs verbatim? This would also allow to note in NEWS that we adjusted these options (referencing the 3.2 note about the future change) while pointing to the manual for the details on the dependency generation.
In any case this should be split to two commits for SVN - one that adjusts the "standard" dependency options and a second one for the new flags. I'd like to review them separately, can you please split the PR into two (obviously the new flags would not base on the original target but on the deps branch)?
Friendly ping @lefessan
@lefessan I've did a quick browser-only merge, can you please review, adjust as needed and check the review comment above?
Thanks for the rebase. As guessed before, noted in the review and now seen in the CI: we have issues at least on Win32 as follows:
# -*- compilation -*-
28. used_binaries.at:1119: testing output dependencies ...
../../tests/used_binaries.at:1138: mkdir -p sub
../../tests/used_binaries.at:1144: $COMPILE_ONLY prog.cob
../../tests/used_binaries.at:1146: $COMPILE_ONLY -M prog.cob prog.cob
--- - 2024-03-12 14:25:51.094447400 +0000
+++ /d/a/gnucobol/gnucobol/_build/tests/testsuite.dir/at-groups/28/stdout 2024-03-12 14:25:51.042599800 +0000
@@ -2,12 +2,12 @@
prog.cob \
COPY1.CPY \
COPY2.CPY \
- sub/COPY3.CPY
+ sub\COPY3.CPY
prog.o: \
prog.cob \
COPY1.CPY \
COPY2.CPY \
- sub/COPY3.CPY
+ sub\COPY3.CPY
I think all make implementations support / paths, in this case we may use these here; otherwise the test may get a conditional to change the expected result for Win32 to backslash or we go with adjusting the output of (.)\(.) to $1/$2.
So, in this version, I am using the COB_IS_RUNNING_IN_TESTMODE
variable to replace backslashes by slashes in the compiler output, so that tests have the same results on Unix and Windows.
Should
-MM
and-MMD
be added, ignoring the copybooks inCOB_COPY_DIR
(currently extfh and screenio)?
I don't think it is worth it. Though they are easy to implement (by modifying the cb_copy_find_file
in pplex.l
to set a flag when using the COB_COPY_DIR
entry of cb_include_list
), I don't see any reason why the external tool that would use the dependencies would need to remove system dependencies, or wouldn't be able to do it by itself. Adding low value features makes the code harder to maintain for little benefit.
This version has a NEWS
section and a new section in gnucobol.texi
for dependency options.
@GitMensch I think I have replied and/or fixed all your points, is it for merge ?
-fcopybook-deps
outputs only copybook names instead of file paths.-fcopybook-deps
also forces-E -foneline-deps -MT=copybooks
, disables errors on missing copybooks and remove output on stdout.