Skip to content

Mark protobuf include path as system include#23012

Closed
bddppq wants to merge 1 commit intopytorch:masterfrom
bddppq:ci-all/protobuf-system-include
Closed

Mark protobuf include path as system include#23012
bddppq wants to merge 1 commit intopytorch:masterfrom
bddppq:ci-all/protobuf-system-include

Conversation

@bddppq
Copy link
Contributor

@bddppq bddppq commented Jul 18, 2019

To suppress (many) compiler warnings from protobuf headers

@pytorchbot pytorchbot added the module: build Build system issues label Jul 18, 2019
@bddppq bddppq force-pushed the ci-all/protobuf-system-include branch from b640f4b to 90d53a8 Compare July 18, 2019 06:22
To suppress compiler warnings from protobuf headers

[ci all]
@bddppq bddppq force-pushed the ci-all/protobuf-system-include branch from 90d53a8 to 22c2c0e Compare July 18, 2019 06:29
@xuhdev
Copy link
Collaborator

xuhdev commented Jul 18, 2019

I think it might be better to try to upgrade protobuf first, but the codebase does not seem to work with the latest protobuf. See #22595

@bddppq
Copy link
Contributor Author

bddppq commented Jul 18, 2019

@xuhdev Yeah I have tried to upgrade protobuf to v3.8.0 previously but have run into the same test failures #22437. But I think this change doesn't have dependency on the upgrade though?

@bddppq
Copy link
Contributor Author

bddppq commented Jul 18, 2019

conda CI have passed

@bddppq bddppq requested review from kostmo and pjh5 July 18, 2019 08:29
@bddppq
Copy link
Contributor Author

bddppq commented Jul 18, 2019

@xuhdev Any concerns on me landing this PR before the protobuf upgrade?

@pjh5
Copy link
Contributor

pjh5 commented Jul 18, 2019

@bddppq It doesn't look like this breaks anything, so I have no concerns. This looks like a good change regardless of the protobuf version. I'd land now.

@xuhdev
Copy link
Collaborator

xuhdev commented Jul 18, 2019

I have no real concerns at this point of time.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@bddppq is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@bddppq merged this pull request in a2b3403.

zdevito pushed a commit to zdevito/ATen that referenced this pull request Jul 18, 2019
Summary:
To suppress (many) compiler warnings from protobuf headers
Pull Request resolved: pytorch/pytorch#23012

Differential Revision: D16364573

Pulled By: bddppq

fbshipit-source-id: adbc4921e29389131d43e7bcc1e6fcba19450c76
soumith added a commit that referenced this pull request Oct 9, 2019
soumith added a commit that referenced this pull request Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged module: build Build system issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants