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.

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

Override CocoaPods module to lowercase #6464

Merged
merged 3 commits into from
Aug 6, 2019
Merged

Override CocoaPods module to lowercase #6464

merged 3 commits into from
Aug 6, 2019

Conversation

paulb777
Copy link
Contributor

@paulb777 paulb777 commented Aug 2, 2019 •
edited
Loading

Fix #3218

Implement suggestion from @BugsBunnyBR at #3218 (comment) to use module_name in podspec to make the module name consistent with the directory structure.

Presumably another change is needed to create the generated files properly?

@paulb777 paulb777 mentioned this pull request Aug 2, 2019
Closed
@TeBoring TeBoring requested a review from thomasvl August 2, 2019 19:23
@thomasvl
Copy link
Contributor

thomasvl commented Aug 5, 2019

Presumably another change is needed to create the generated files properly?

https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/compiler/objectivec/objectivec_helpers.cc#L984

Should be the change to do it.

Then running generate_descriptor_proto.sh at the root of the tree should regenerate the files.

Having said that, I believe this still counts as a breaking api change because anyone that directly imports the headers in their code will have to update things because of the new module name? If they only import their own generate files I guess it should slide by without issue?

@paulb777
Copy link
Contributor Author

paulb777 commented Aug 5, 2019

I'm not sure it's a breaking API change. At least with FirebaseMessaging, the warnings go away even though the imports are uppercase like in https://github.com/firebase/firebase-ios-sdk/blob/master/Firebase/Messaging/FIRMessagingSecureSocket.m#L19

@thomasvl
Copy link
Contributor

thomasvl commented Aug 5, 2019

I'm not sure it's a breaking API change. At least with FirebaseMessaging, the warnings go away even though the imports are uppercase...

Lovely, just goes to show even more how Apple's impl of this warning wasn't complete. Since that case should be exactly what causes this, no?

@blasten blasten mentioned this pull request Aug 5, 2019
Closed
@paulb777
Copy link
Contributor Author

paulb777 commented Aug 5, 2019

Yep. It looks like as long as the module name is lowercase, imports compile without warning whether they reference it with an upper-case or lower-case module name.

@paulb777 paulb777 mentioned this pull request Aug 6, 2019
Closed
thomasvl
thomasvl approved these changes Aug 6, 2019
View reviewed changes
Copy link
Contributor

@thomasvl thomasvl left a comment

Choose a reason for hiding this comment

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

🤞 this doesn't cause issues when the protobuf team cuts the next release.

@thomasvl thomasvl merged commit 479ba82 into protocolbuffers:master Aug 6, 2019
@juliocbcotta
Copy link

@thomasvl any idea of when this will be in a cocoapods release?

@thomasvl
Copy link
Contributor

@thomasvl any idea of when this will be in a cocoapods release?

I'm not completely sure (I don't work directly on the team, I'm just responsible for the ObjC part). Hopefully they will have a release in the not too distant future.

@penacristian
Copy link

I just updated to 3.9.2 and I still get the protobuf case error. Any ideas?

@thomasvl
Copy link
Contributor

thomasvl commented Oct 3, 2019

3.9.2 just had a tweak for something that was causing crashes on 32bit arm devices. The packaging change is in 3.10.x, so will be a GM soon.

@penacristian
Copy link

Any timeline estimation?

@brownoxford
Copy link

I just updated Protobuf pod from 3.9.2 to 3.10.0 and this issue has been resolved for me.

@alexxsanchezm alexxsanchezm mentioned this pull request Oct 28, 2019
Closed
10 tasks
@ledyba-z ledyba-z mentioned this pull request Dec 11, 2019
Closed
thomasvl added a commit to thomasvl/protobuf that referenced this pull request Jan 27, 2020
@thomasvl
Revert "Override CocoaPods module to lowercase (protocolbuffers#6464)"
387453e 
This reverts commit 479ba82.
@thomasvl thomasvl mentioned this pull request Feb 4, 2020
Merged
thomasvl added a commit to thomasvl/protobuf that referenced this pull request Feb 6, 2020
@thomasvl
Revisit how the WKTs are bundled with ObjC.
6b4b28c 
There are have been a few issues around people using case sensitive file systems
what Xcode/clang does when looking at the paths. In attempts to solve one set of
warnings, new warnings/errors happened in different setup. So, to hopefully put
these problem away for got, move the WKTs to be at the same level as the other
headers.

- Revert "Override CocoaPods module to lowercase (protocolbuffers#6464)"
  This reverts commit 479ba82.
- Move WKTs to the objectivec directory and make the old headers shim back to
  the new locations.
- Update objectivec/generate_well_known_types.sh to check them one at a time
  and to deal with the new locations for them.

Fixes protocolbuffers#6803
thomasvl added a commit that referenced this pull request Feb 10, 2020
@thomasvl
Revisit how the WKTs are bundled with ObjC.
56c48ae 
There are have been a few issues around people using case sensitive file systems
what Xcode/clang does when looking at the paths. In attempts to solve one set of
warnings, new warnings/errors happened in different setup. So, to hopefully put
these problem away for got, move the WKTs to be at the same level as the other
headers.

- Revert "Override CocoaPods module to lowercase (#6464)"
  This reverts commit 479ba82.
- Move WKTs to the objectivec directory and make the old headers shim back to
  the new locations.
- Update objectivec/generate_well_known_types.sh to check them one at a time
  and to deal with the new locations for them.

Fixes #6803
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasvl thomasvl thomasvl approved these changes

Assignees
No one assigned
Labels
cla: yes objective-c
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Xcode reports non-portable path to header
7 participants
@paulb777 @thomasvl @juliocbcotta @penacristian @brownoxford @googlebot @protobuf-kokoro