tuxji commented on a change in pull request #681:
URL: https://github.com/apache/daffodil/pull/681#discussion_r752618483
##########
File path: .gitattributes
##########
@@ -13,4 +13,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+# Do not include KEYS in archived source releases
/KEYS export-ignore
+
+# Do not convert to CRLF on Windows since tests require LF files
+/**/runtime2/examples/** text=auto eol=lf
Review comment:
OK, we are discussing two separate issues now:
1. Should test_updateGeneratedCodeExamples fail every time or only the first
time?
2. Should the C code generator write a) only LFs or b) System.lineSeparator
into generated code files regardless of which line endings are used in source
files?
For 1), I don't think we can separate the test and the update without adding
an extra step (the update command) to the code contributor's workflow. Instead
of simply running "sbt test" until no test fails, the contributor will have to
run "sbt genExamples" and then "sbt test" again to verify the tests pass now.
Do we really want to add more friction to the code contributor's workflow?
I say the convenience of combining the test and the update outweighs the
strangeness of the test failing only the first time. The test may seem like it
will fail only once, but it will invariably fail every time when run on a fresh
checkout of the repository on a GitHub Actions runner. The test in CI will
prevent anyone from committing a change to the code generator without running
"sbt test" first.
At the same time, I do think we need to expand the assert's message so that
any code contributor will understand what is going on, especially if they're
merely publishing a new daffodil release (even changing daffodil's version in
build.sbt without running "sbt test" will make the CI job fail). "Examples
have been updated!" is too terse for GitHub Actions; we probably should write
something like "You need to update the examples! Run `sbt test` and commit the
updated example code files." Suggestions for better phrasing would be welcome.
For 2), I could make an even smaller change if we want the C code generator
to write only LFs or System.lineSeparator regardless of source files' line
endings. I would modify CodeGeneratorState's generatedCodeHeader and
generatedCodeFile functions to replace CRLF and LF before returning the final
strings to be written out to files. That's only two new lines of code, but we
would have to replace LF with System.lineSeparator before we could remove the
new line from .gitattributes. Unfortunately, if we remove the new line from
.gitattributes, the test still could fail in a code contributor's checkout due
to the contributor's git config settings. The test will work in GitHub
Actions, but it won't work for all code contributors running it because they
could have checked out the example code files on Windows with either LF or CRLF
line endings depending on their settings. The best thing to do would be to
leave the new line but change it to `* text=auto`, which causes git to check
out all text-like files with System.lineSeparator line endings regardless of
git config settings. Then the examples and the generated files will match each
other - they'll always be System.lineSeparator on GitHub Actions and in
anyone's checkout. Files that are already CRLF in git will remain CRLF on
checkout with this new line as well.
John (@jw3) or anyone else, please weigh in too (I've been waiting a long
time for a second +1).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
