Skip to content

Add macOS build support#872

Merged
tuxji merged 1 commit intoapache:mainfrom
pkatlic:daffodil-2744-macos-build
Nov 10, 2022
Merged

Add macOS build support#872
tuxji merged 1 commit intoapache:mainfrom
pkatlic:daffodil-2744-macos-build

Conversation

@pkatlic
Copy link
Contributor

@pkatlic pkatlic commented Nov 10, 2022

Add build documentation and update code to support building on macOS. Rename existing stack_t struct to avoid conflict with built-in type definition on macO. Use int64 and uint64 primitives for integer types to fix compilation warnings. Add portable endian header file for macOS endian definitions, as these are not located in endian.h. This was tested on macOS 10.15.7.

DAFFODIL-2744

BUILD.md: Add macOS dependency installation to build documentation.

stack.h: Rename stack_t struct to c_stack_t in order to avoid naming conflict with macOS types.

stack.c: Rename stack_t struct to c_stack_t in order to avoid naming conflict with macOS types.

xml_writer.h: Rename stack_t struct to c_stack_t in order to avoid naming conflict with macOS types.

xml_writer.c: Use PRIi64 and PRIu64 integer types to avoid compilation warnings on macOS.

p_endian.h: Add portable endian header file for including proper endian header files and defines on macOS.

parsers.c: Import portable endian header file.

unparsers.c: Import portable endian header file.

Add build documentation and update code to support building on macOS.
Rename existing stack_t struct to avoid conflict with built-in type
definition on macO. Use int64 and uint64 primitives for integer types
to fix compilation warnings. Add portable endian header file for macOS
endian definitions, as these are not located in endian.h. This was
tested on macOS 10.15.7.

DAFFODIL-2744

BUILD.md: Add macOS dependency installation to build documentation.

stack.h: Rename stack_t struct to c_stack_t in order to avoid naming
conflict with macOS types.

stack.c: Rename stack_t struct to c_stack_t in order to avoid naming
conflict with macOS types.

xml_writer.h: Rename stack_t struct to c_stack_t in order to avoid
naming conflict with macOS types.

xml_writer.c: Use PRIi64 and PRIu64 integer types to avoid compilation
warnings on macOS.

p_endian.h: Add portable endian header file for including proper
endian header files and defines on macOS.

parsers.c: Import portable endian header file.

unparsers.c: Import portable endian header file.
Copy link
Member

@stevedlawrence stevedlawrence left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 Thanks! Looks good to me.

I think this is a question for other devs: should we consider adding a GitHub action for macOS to ensure this continues to work?

Copy link
Contributor

@tuxji tuxji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

I reviewed the build logs and found two small issues (both are unrelated to this pull request, on which Peter did an excellent job :).

The Build Documentation step generates 1 warning; I am sure a trivial change can (and should be done) to fix this warning.

[warn] /home/runner/work/daffodil/daffodil/daffodil-udf/src/main/java/org/apache/daffodil/udf/UserDefinedFunctionProvider.java:48:3: Could not find any member to link for "ReflectiveOperationException".
[warn]   /**
[warn]   ^
[warn] one warning found

The Run Integration Tests step now generates 3 warnings; these warnings were not in the last CI job run before the integration tests were refactored for clarity and speed. We should investigate these warnings since they may indicate a problem with the new API in four of the blob tests.

[warn] Test assumption in test org.apache.daffodil.blob.TestBlob.test_blob_backtracking_streaming_fail failed: org.junit.AssumptionViolatedException: large test input file must be manually generated, took 0.003 sec
[warn] Test assumption in test org.apache.daffodil.blob.TestBlob.test_1MB_blob failed: org.junit.AssumptionViolatedException: large test input file must be manually generated, took 0.0 sec
[warn] Test assumption in test org.apache.daffodil.blob.TestBlob.test_2GB_blob failed: org.junit.AssumptionViolatedException: large test input file must be manually generated, took 0.0 sec
[warn] Test assumption in test org.apache.daffodil.blob.TestBlob.test_blob_backtracking failed: org.junit.AssumptionViolatedException: large test input file must be manually generated, took 0.0 sec
[info] Test run org.apache.daffodil.blob.TestBlob finished: 0 failed, 0 ignored, 4 total, 0.004s

@mbeckerle
Copy link
Contributor

I think this is a question for other devs: should we consider adding a GitHub action for macOS to ensure this continues to work?

Yes, but not as part of this ticket, but another one about specifically setting that up.

@stevedlawrence
Copy link
Member

The Build Documentation step generates 1 warning

There's lots of discussions on the internet about this error, with I think no real resolution. I think this might be a known issue.

The Run Integration Tests step now generates 3 warnings;

This was a change made in the recent integration test refactoring. We used to just have the blob tests commented out because they require manually generating large files to test. But because they were commented out, they were easy to forget about. To avoid that, I added some JUnit assumeTrue calls so that the tests would be skipped if those generated files didn't exist. That way the tests still pass, but we get a clear reminder that those tests exist and were skipped. We don't need to run these very often because blob logic doesn't change much, but they're good to know have be periodically reminded of.

@stevedlawrence
Copy link
Member

I think this is a question for other devs: should we consider adding a GitHub action for macOS to ensure this continues to work?

Yes, but not as part of this ticket, but another one about specifically setting that up.

Agreed it can be done separately. I've created DAFFODIL-2747 for this.

@tuxji
Copy link
Contributor

tuxji commented Nov 10, 2022

I think this is a question for other devs: should we consider adding a GitHub action for macOS to ensure this continues to work?

Peter and I certainly can modify the GitHub Actions CI workflow if anyone else would like macOS support. If we do so, I propose that we run only one job on the macOS runner (that is, we don't test Java 8, 11, and 17 since that would be redundant with the Linux and Windows runners already testing Java 8, 11, and 17). I have been using Java 11 as my machine's Java runtime up to now, but googling makes me think our choice really should be between Java 8 (if we want to support the most widely used Java runtime) or Java 17 (if we want to support the current LTS version and Java 17 was released over 1 year ago, so it's had enough time to fix bugs and become a stable runtime). Shall we test only Java 17 on the macOS runner?

@tuxji tuxji merged commit e34f32c into apache:main Nov 10, 2022
@tuxji
Copy link
Contributor

tuxji commented Nov 10, 2022

I'll move our discussion about Java runtimes to DAFFODIL-2747.

@pkatlic pkatlic deleted the daffodil-2744-macos-build branch September 7, 2023 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants