[native] add CXX header files to Xcode project
ClosedPublic
Actions

Authored by varun on Sep 8 2022, 9:46 AM.

Details

Reviewers

atul
• jon
tomek

Commits

rCOMM3e252805c510: [native] add CXX header files to Xcode project

Summary

There doesn't seem to be a straightforward way to add generated header files to our Xcode project without checking them in. This change lets users easily call rust functions declared in the lib.rs.h header file.

Depends on D4953

Test Plan

Built the mobile app in Xcode

Diff Detail

Repository

rCOMM Comm

Branch

identity-client (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

varun created this revision.Sep 8 2022, 9:46 AM

Herald added subscribers: • abosh, tomek, ashoat. · View Herald TranscriptSep 8 2022, 9:46 AM

Harbormaster completed remote builds in B12021: Diff 16504.Sep 8 2022, 9:57 AM

varun requested review of this revision.Sep 8 2022, 9:57 AM

I would like to avoid doing grpc/grpc_client and instead just have it under grpc/, after https://phab.comm.dev/D5045, only the client will remain in the folder

This revision now requires changes to proceed.Sep 8 2022, 10:22 AM

Unstaged changes in working copy:
    native/cpp/CommonCpp/grpc/grpc_client/cxx.h
    native/cpp/CommonCpp/grpc/grpc_client/lib.rs.h

Let's add these to .gitignore

(Accepting, but should address @jon's feedback and get his acceptance before landing)

Can we use the same approach as we do for other generated grpc files? E.g. mark them accordingly so that they don't show up on review.

In D5084#148511, @atul wrote:
Unstaged changes in working copy:
    native/cpp/CommonCpp/grpc/grpc_client/cxx.h
    native/cpp/CommonCpp/grpc/grpc_client/lib.rs.h
Let's add these to .gitignore

(Accepting, but should address @jon's feedback and get his acceptance before landing)

we actually don't want to .gitignore these. we need to check them in for now

probably need to fix the formatting of the generated files or something if you're seeing unstaged changes?

mark header files as generated

hmmm now the project.pbxproj file is showing up as generated on phabricator (which it is, but we still want phabricator to show it). is it because the word @generated appears?

In D5084#148472, @jon wrote:

I would like to avoid doing grpc/grpc_client and instead just have it under grpc/, after https://phab.comm.dev/D5045, only the client will remain in the folder

i think this makes sense as a separate change because i'll be moving a bunch of files and it'll clutter this diff. what do you think?

Harbormaster completed remote builds in B12062: Diff 16562.Sep 9 2022, 12:50 PM

varun added a child revision: D5104: [native] restructure gRPC client cargo project as general purpose library crate for native.Sep 9 2022, 1:35 PM

https://phab.comm.dev/D5084#148472 is still applicable

This revision now requires changes to proceed.Sep 9 2022, 2:01 PM

In D5084#149052, @jon wrote:

https://phab.comm.dev/D5084#148472 is still applicable

I replied to this comment above :)

i think this makes sense as a separate change because i'll be moving a bunch of files and it'll clutter this diff. what do you think?

with the generated file collapse, it will just be the paths which change. unless your manually editing the files.

Either way, the other revision which moved out the protos has landed, so I think we should avoid creating another directory.

This revision now requires changes to proceed.Sep 9 2022, 3:34 PM

In D5084#149035, @varun wrote:

hmmm now the project.pbxproj file is showing up as generated on phabricator (which it is, but we still want phabricator to show it). is it because the word @generated appears?

Yes, that's the reason. We should use comm/shared/scripts/mark-generated.sh script instead of doing it directly.

In D5084#149188, @tomek wrote:

In D5084#149035, @varun wrote:

hmmm now the project.pbxproj file is showing up as generated on phabricator (which it is, but we still want phabricator to show it). is it because the word @generated appears?

Yes, that's the reason. We should use comm/shared/scripts/mark-generated.sh script instead of doing it directly.

the mark-generated file appears to be specific to the gRPC codegen'd files. it's doing a replacement, whereas we need to prefix the keyword in the cxx files. i'm going to undo the latest change and defer this https://linear.app/comm/issue/ENG-1810/improve-rust-library-integration-in-xcode

In D5084#149061, @jon wrote:

i think this makes sense as a separate change because i'll be moving a bunch of files and it'll clutter this diff. what do you think?

with the generated file collapse, it will just be the paths which change. unless your manually editing the files.

Either way, the other revision which moved out the protos has landed, so I think we should avoid creating another directory.

the grpc_client folder existed prior to this diff. it's the folder that contains the cargo project for the library that i'm introducing in native. we can definitely reduce some nesting now that your restructuring diffs have landed, but it doesn't make sense to me to address that here. lmk if i'm misunderstanding something