Skip to content

Cpp target: No building tests/samples for external utfcpp #3040

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 8, 2021
Merged

Cpp target: No building tests/samples for external utfcpp #3040

merged 6 commits into from
Apr 8, 2021

Conversation

bigerl
Copy link
Contributor

@bigerl bigerl commented Jan 19, 2021 •
edited
Loading

It is unnecessary that each time the external dependency utfcpp is pulled, its tests are build and run. Also its samples are unnecessarily generated.
I suggest to use the provided cmake to disable this behavior.
You can find the documentation of the cmake options here: https://github.com/nemtrif/utfcpp/blob/v3.1.1/CMakeLists.txt#L10-L12

Let's make this build a little bit more green and not use unnecessary resources.

@bigerl bigerl changed the title Patch 1 Cpp target: No building tests/samples for external utfcpp Jan 19, 2021
mike-lischke
mike-lischke approved these changes Apr 8, 2021
View reviewed changes
Copy link
Member

@mike-lischke mike-lischke left a comment

Choose a reason for hiding this comment

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

@parrt Another C++ build change ready to be merged. This might create a conflict with the other build change for utf8cpp, but is the older one, so merge this first please. The other patch might have to be updated then.

@mike-lischke
Copy link
Member

@bigerl Please fix the contributors file in your patch.

@mike-lischke mike-lischke mentioned this pull request Apr 8, 2021
Open
@parrt parrt merged commit b71f689 into antlr:master Apr 8, 2021
@mike-lischke
Copy link
Member

@bigerl Your patch broke the build with this error:

Building ANTLR4 C++ runtime (if necessary) at /Users/macosx-ci-4/actions-runner/_work/antlr4/antlr4/runtime-testsuite/target/classes/Cpp
Cloning into 'utfcpp'...
fatal: could not create work tree dir 'utfcpp': File exists
fatal: destination path 'utfcpp' already exists and is not an empty directory.
fatal: destination path 'utfcpp' already exists and is not an empty directory.
CMake Error at /Users/macosx-ci-4/actions-runner/_work/antlr4/antlr4/runtime-testsuite/target/classes/Cpp/runtime/utfcpp-prefix/tmp/utfcpp-gitclone.cmake:31 (message):
Failed to clone repository: 'git://github.com/nemtrif/utfcpp'

Can you fix that?

@bigerl
Copy link
Contributor Author

bigerl commented Apr 9, 2021 •
edited
Loading

@mike-lischke

I've checked the delta between my fork's master and the current master. There have been several changes to cmake files since I created the pull request which might influence the behavior I patched.
Also there is additional CI code now which might fail.

I do not have time to look into that immediately. So I would suggest the following:

  • revert the merge of this pull-request
  • I'll fork the current master and apply and test the patch again; ETA 2 weeks
  • When I am done, I'll create another pull request.

@mike-lischke mike-lischke mentioned this pull request Apr 9, 2021
Merged
@mike-lischke
Copy link
Member

OK @bigerl I created a patch to revert yours and the builds are green again. Just create a new PR for your case when you are ready.

@bigerl bigerl mentioned this pull request Apr 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@mike-lischke mike-lischke mike-lischke approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
comp:build target:cpp
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bigerl @mike-lischke @parrt