Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prep work for running test suite on macos-latest #248

Merged
merged 4 commits into from
Sep 12, 2024
Merged

Prep work for running test suite on macos-latest #248

merged 4 commits into from
Sep 12, 2024

Conversation

jnikula
Copy link
Owner

@jnikula jnikula commented Aug 31, 2024

GitHub allows running workflows on macos in addition to ubuntu. I'd potentially like to start running tests on it, but there are some hiccups. I'm not quite there yet with fixing everything in setting up the workflow, but here are two small fixes for starters.

EDIT: This is expanding quite a bit...

EDIT: This + the actual workflow is #249. I'm keeping these separate as this is fairly straightforward prep work, and the workflow doesn't pass.

EDIT: And back to basics. This is just the good stuff now.

@jnikula jnikula requested a review from BrunoMSantos August 31, 2024 13:25
@jnikula jnikula force-pushed the macos branch 2 times, most recently from 463937c to 7c5feb3 Compare September 1, 2024 11:56
@jnikula jnikula mentioned this pull request Sep 1, 2024
@jnikula jnikula force-pushed the macos branch 2 times, most recently from 11dbb3d to dacae5d Compare September 11, 2024 13:43
Copy link
Collaborator

@BrunoMSantos BrunoMSantos left a comment

Choose a reason for hiding this comment

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

Good goal and it's a tentative approve from me, but see my other comments 1st.

test/test_parser.py Show resolved Hide resolved
test/test_cautodoc.py Outdated Show resolved Hide resolved
The BSD find requires the path even for current directory. No harm in
providing it on GNU find too.
On macOS, the temporary directories created for Sphinx srcdir contains a
symlink /var -> /private/var. The error messages returned from Sphinx
have the symlinks eliminated, and the path removal from error messages
fails, leading to test failures. Pass the srcdir through
os.path.realpath() to eliminate symlinks everywhere, so the filename
removal works.
This will be useful for parser tests, as we'll be able to reuse this
instead of duplicating the logic for filename or line being None.
The parser may return None for clang diagnostics filename if there isn't
one. We already handle this gracefully in get_message(), so reuse that
instead of duplicating the logic, now that there's support for
basename=True.
@jnikula
Copy link
Owner Author

jnikula commented Sep 12, 2024

Switched to using os.path.realpath(srcdir) to handle the symlinks, which is a more general approach I think. Also improved the commit messages around get_message() use slightly.

Testing the macos workflows in a private repo, I'm getting pretty close to passing the tests, but not quite.

@jnikula
Copy link
Owner Author

jnikula commented Sep 12, 2024

With the xcode (or whatever the "stock" toolchain is called) version of libclang, I'm hitting e.g.:

E         + WARNING: using-alias.cpp:2: alias declarations are a C++11 extension
E         + WARNING: using-alias.cpp:5: alias declarations are a C++11 extension
E         + WARNING: using-alias.cpp:9: alias declarations are a C++11 extension
E         + WARNING: using-alias.cpp:12: variadic templates are a C++11 extension
E         + WARNING: using-alias.cpp:13: alias declarations are a C++11 extension
E         + WARNING: using-alias.cpp:16: variadic templates are a C++11 extension
E         + WARNING: using-alias.cpp:17: alias declarations are a C++11 extension

and

E         + ERROR: struct-class.cpp:26: unknown type name 'constexpr'
E         + /Users/runner/work/github-actions-playground/github-actions-playground/test/cpp/struct-class.cpp:23: WARNING: Error in declarator or parameters-and-qualifiers
E         + If pointer to member declarator:
E         +   Invalid C++ declaration: Expected identifier in nested name. [error at 48]
E         +     public static constexpr  static_constexpr_member
E         +     ------------------------------------------------^
E         + If declarator-id:
E         +   Invalid C++ declaration: Expected identifier in nested name. [error at 48]
E         +     public static constexpr  static_constexpr_member
E         +     ------------------------------------------------^
E         +

and

E         + ERROR: class.cpp:28: unknown type name 'constexpr'
E         + ERROR: class.cpp:34: expected ';' at end of declaration list
E         + ERROR: class.cpp:37: unknown type name 'constexpr'
E         + WARNING: class.cpp:70: deleted function definitions are a C++11 extension
E         + WARNING: class.cpp:73: 'override' keyword is a C++11 extension
E         + WARNING: class.cpp:91: defaulted function definitions are a C++11 extension

and

E         + ERROR: namespace.cpp:26: unknown type name 'constexpr'
E         + WARNING: namespace.cpp:52: scoped enumerations are a C++11 extension

and

E         + ERROR: class.cpp:28: unknown type name 'constexpr'
E         + ERROR: class.cpp:34: expected ';' at end of declaration list
E         + ERROR: class.cpp:37: unknown type name 'constexpr'
E         + WARNING: class.cpp:70: deleted function definitions are a C++11 extension
E         + WARNING: class.cpp:73: 'override' keyword is a C++11 extension
E         + WARNING: class.cpp:91: defaulted function definitions are a C++11 extension
E         + /Users/runner/work/github-actions-playground/github-actions-playground/test/cpp/class.cpp:25: WARNING: Error in declarator or parameters-and-qualifiers
E         + If pointer to member declarator:
E         +   Invalid C++ declaration: Expected identifier in nested name. [error at 48]
E         +     public static constexpr  static_constexpr_member
E         +     ------------------------------------------------^
E         + If declarator-id:
E         +   Invalid C++ declaration: Expected identifier in nested name. [error at 48]
E         +     public static constexpr  static_constexpr_member
E         +     ------------------------------------------------^

Additionally, there are some failures to find C++ headers. #252 improved the situation in that the paths aren't wrong, but still issues.

@jnikula
Copy link
Owner Author

jnikula commented Sep 12, 2024

With the homebrew version of libclang, I think all of the issues in both C and C++ boil down to inability to find correct C headers. Looks like the C++ headers are found just fine, but the C++ headers including C headers also hit issues. For example, test_parser[cpp/template]:

  + CRITICAL: wchar.h:89: 'stdarg.h' file not found

but template.cpp only includes <vector> which is found just fine.

Again, I think the situation is slightly better with #252 but I don't know what gives.

@jnikula
Copy link
Owner Author

jnikula commented Sep 12, 2024

Anyway, the pull request at hand is a prerequisite no matter what. #253 has also been very helpful.

@BrunoMSantos
Copy link
Collaborator

Good to merge I think!

@jnikula jnikula merged commit 0a196b2 into master Sep 12, 2024
6 checks passed
@jnikula jnikula deleted the macos branch September 12, 2024 11:32
@jnikula
Copy link
Owner Author

jnikula commented Sep 12, 2024

Good to merge I think!

Thanks, merged!

Can you see the detailed github action results, with logs etc, for my pull requests?

@BrunoMSantos
Copy link
Collaborator

Can you see the detailed github action results, with logs etc, for my pull requests?

I've had a quick look yesterday, but I need to take a bit longer on that one. Haven't seen your latest updates at all though.

@jnikula
Copy link
Owner Author

jnikula commented Sep 12, 2024

I've had a quick look yesterday, but I need to take a bit longer on that one. Haven't seen your latest updates at all though.

Okay, with the prep work merged, I can just send a workflow PR. Mostly I've been playing in a private repo.

@jnikula
Copy link
Owner Author

jnikula commented Sep 12, 2024

@BrunoMSantos Increasingly the wrong place to discuss, but anyway: Looks like we need to specify the C++ standard to use for a lot of tests. The default is different on macos. I just added -std=c++17 to the .yaml files of a bunch of tests. Is that okay, or should we add a default in testenv.py that could be overridden in .yaml?

Still having include issues, and it's driving me nuts. Xcode libclang can't find C++ headers, but homebrew version can. Xcode libclang can find C headers, but homebrew version can't. If one or the other worked, I'd just call it a day.

@BrunoMSantos
Copy link
Collaborator

I only have #249 to go by. Care to make a draft PR with the latest version?

Looks like we need to specify the C++ standard to use for a lot of tests. The default is different on macos. I just added -std=c++17 to the .yaml files of a bunch of tests. Is that okay, or should we add a default in testenv.py that could be overridden in .yaml?

Other than that, assuming a default may be wise (perhaps the difference is in the clang version each system uses by default, in which case it doesn't hurt to be explicit). Adding it in every test case is not so good I think as the test developer may not immediately know he needs to.

Still having include issues, and it's driving me nuts. Xcode libclang can't find C++ headers, but homebrew version can. Xcode libclang can find C headers, but homebrew version can't. If one or the other worked, I'd just call it a day.

Alas I know nothing of macos, but that's weird indeed. How are you testing this? The CI itself or do you have a system on your end? I.e. can I see the output?

@jnikula
Copy link
Owner Author

jnikula commented Sep 12, 2024

I only have #249 to go by. Care to make a draft PR with the latest version?

Sure. #254

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.

2 participants