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

Add disable-exceptions feature to portfile for tbb #16068

Merged
merged 4 commits into from
Apr 8, 2021

Conversation

aggieNick02
Copy link
Contributor

I had forgotten to rev the port version in the last PR. Trying again, starting with a draft PR.

This feature passes exceptions=0 to tbbbuild in the non-windows case (which eventually defines TBB_USE_EXCEPTIONS as 0 in cpp code), and sets TBB_USE_EXCEPTIONS=0 in the vcxproj files in the windows case. I wasn't sure if this counts as exposing an "alternative", but expect a decent number of tbb clients would want this option available. Happy to discuss as this is my first portfile modification attempt.

The effect of the feature is that it removes the try/catch(...) wrappers around user code run by TBB. While these exception facilities can be nice in some cases, their removal allows for much easier debugging of a crash due to an unhandled exception in code that a TBB client provides to a TBB algorithm. With the try/catch(...) wrappers removed, the unhandled exception and crash dump are generated at the point of the thrown exception, versus significantly later in a different thread with the originally throwing thread no longer having the stack from when the exception was thrown.

All triplets should be supported with this feature.

…b_build in the non-windows case, and set TBB_USE_EXCEPTIONS=0 in the vcxproj files in the windows case. This removes the try/catch(...) wrappers around user code run by TBB. While these exception facilities can be nice in some cases, their removal allows for much easier debugging of a crash due to an unhandled exception in code that a TBB client provides to a TBB algortihm. With the try/catch(...) wrappers removed, the unhandled exception and crash dump are generated at the point of the thrown exception, versus significantly later in a different thread with the originally throwing thread no longer having the stack from when the exception was thrown.
@aggieNick02
Copy link
Contributor Author

Are the two CI failures real? I've updated the version in CONTROL and the command the error says to run (x-add-version) no longer exists.

@JackBoosY
Copy link
Contributor

@aggieNick02 Please use command ./vcpkg x-add-version tbb then commit the changes.

@JackBoosY JackBoosY added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Feb 7, 2021
@aggieNick02
Copy link
Contributor Author

Thanks! I didn't realize that gets run after the initial commit. CI running now.

@JackBoosY
Copy link
Contributor

@aggieNick02 Is this PR ready for review?

@aggieNick02
Copy link
Contributor Author

Yes, thank you. I'll move it out of draft state.

@aggieNick02 aggieNick02 marked this pull request as ready for review February 7, 2021 06:06
@JackBoosY
Copy link
Contributor

Does the upstream accept or provide this?

@aggieNick02
Copy link
Contributor Author

Do you mean does TBB already have this functionality? If so, yes, it already has this #if TBB_USE_EXCEPTIONS and exceptions=0 build option (and has for a while), so this is just exposing that capability in the port.

@JackBoosY
Copy link
Contributor

@aggieNick02 One last question: should this feature be a default feature?

@aggieNick02
Copy link
Contributor Author

No, it should not. It changes the default TBB exception behavior, so users should have to opt-in if they want it.

@JackBoosY
Copy link
Contributor

Tested all features succesfully on x86-windows and x64-windows-static.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Feb 7, 2021
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

This seems firmly within the realm of alternatives:

  1. It would be inappropriate for another port to "depend" on this
  2. Changing this has significant, breaking behavior changes

Therefore, the two approaches available to set this are:

  1. Expect the end-user to add a setting to their triplet file (something like set(TBB_USE_EXCEPTIONS OFF))
  2. Expect the end-user to use overlays to modify the portfile.cmake contents.

During normal execution, either of these could be advertised via message:

message("TBB is built with exceptions by default. To disable exception support, add `set(TBB_USE_EXCEPTIONS OFF)` to your triplet.")

@aggieNick02
Copy link
Contributor Author

Thanks. Overlays is what I'm doing right now to accomplish this locally. I hadn't thought about the implications of another port depending on this, I see why that would be problematic.

I'm pretty green with makefiles/etc, so figuring out what to change where to get this working with the overlays did take me a little while. If there is a way to make this easier for others, it would be nice. The fact that tbb is built via vs projects on windows and cmake elsewhere made it a little more involved.

So I'd hope of the two approaches available, option 2 you listed isn't what anyone who wants to disable tbb exceptions has to do. Option 1 would be nice. To do that, would I just revert the change to the CONTROL, eliminate my if/then/else in the portfile for checking the feature presence and setting DISABLE_EXCEPTIONS, and then instead I can just have a TBB_DISABLE_EXCEPTIONS that is checked/used in the portfile?

@JackBoosY JackBoosY removed info:reviewed Pull Request changes follow basic guidelines requires:author-response labels Feb 9, 2021
@ras0219-msft
Copy link
Contributor

Yep! And either a comment or a message() that helps users find it and figure out what to do.

@aggieNick02
Copy link
Contributor Author

dumb question (first public PR) - do you prefer i add commits with the requested changes (so there would be the first 2 commits plus more for the fixes), or just make a new commit starting from scratch and force push that for review (so there is only one commit)?

@JackBoosY
Copy link
Contributor

JackBoosY commented Feb 10, 2021

@aggieNick02 You can push more commits, don't worry about that.

@aggieNick02
Copy link
Contributor Author

Think I've got all the changes lined up now. Thanks for the patient feedback. Just let me know if anything still looks amiss.

@JackBoosY
Copy link
Contributor

cc @ras0219 for review this PR again.

Copy link
Contributor

@ras0219 ras0219 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Feb 19, 2021
@aggieNick02
Copy link
Contributor Author

@ras0219 Is there anything still needed for this PR to get merged?

@JackBoosY
Copy link
Contributor

Ping @vicroms for merge this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants