Skip to content

Conversation

@darinf
Copy link

@darinf darinf commented May 3, 2024

Changes:

  • Adds FIREBASE_INCLUDE_{FUNCTIONS,STORAGE} defines to the cmake build.
  • Includes corresponding header files in the nuget package.
  • Includes generated libs in the nuget package.
  • Removes upb.lib which appears to no longer be generated.

@darinf darinf marked this pull request as ready for review May 3, 2024 23:52
@darinf darinf requested review from compnerd, hyp and kendalharland May 3, 2024 23:53
@darinf
Copy link
Author

darinf commented May 3, 2024

I think this is sufficient but also not sure how to validate this. Would appreciate any guidance. Thanks!

@kendalharland
Copy link
Member

+1 from me, but will defer to @hyp and @compnerd as I'm still ramping up on this project's build definition.

Copy link

@hyp hyp left a comment

Choose a reason for hiding this comment

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

That looks reasonable to me

Copy link
Collaborator

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

I don't remember if we need to manually trigger the new build - so if it doesn't get another build out after this is merged, we might need to trigger it manually before you can update the consumer of the nuget.

@compnerd
Copy link
Collaborator

compnerd commented May 4, 2024

I think this is sufficient but also not sure how to validate this. Would appreciate any guidance. Thanks!

I think that the best way to validate this might be to do a one-off CI run of the build and check the nuget contents. Its not a great solution, but it at least should give you some level of confidence.

Might be worthwhile to trigger CI runs on Windows as well for future changes. WDYT @darinf?

@darinf
Copy link
Author

darinf commented May 6, 2024

I think this is sufficient but also not sure how to validate this. Would appreciate any guidance. Thanks!

I think that the best way to validate this might be to do a one-off CI run of the build and check the nuget contents. Its not a great solution, but it at least should give you some level of confidence.

Might be worthwhile to trigger CI runs on Windows as well for future changes. WDYT @darinf?

That all sounds good, but ... I'm not sure how to do so. Any pointers?

@darinf
Copy link
Author

darinf commented May 6, 2024

I think this is sufficient but also not sure how to validate this. Would appreciate any guidance. Thanks!

I think that the best way to validate this might be to do a one-off CI run of the build and check the nuget contents. Its not a great solution, but it at least should give you some level of confidence.
Might be worthwhile to trigger CI runs on Windows as well for future changes. WDYT @darinf?

That all sounds good, but ... I'm not sure how to do so. Any pointers?

Maybe it is this:

Screenshot 2024-05-05 at 5 35 40 PM

@compnerd
Copy link
Collaborator

compnerd commented May 6, 2024

Yes, you can manually trigger the job from the actions tab and selecting the job.

@darinf
Copy link
Author

darinf commented May 6, 2024

Yes, you can manually trigger the job from the actions tab and selecting the job.

Cool, unfortunately it seems to have failed:
https://github.com/thebrowsercompany/firebase-cpp-sdk/actions/runs/8962331804

I'm struggling to see any error in that log output though :-/

@compnerd
Copy link
Collaborator

compnerd commented May 6, 2024

2024-05-06T00:46:09.3703288Z D:\a\firebase-cpp-sdk\firebase-cpp-sdk\BinaryCache\firebase\external\src\curl\lib\nonblock.c(89,1): error C1189: #error: "no non-blocking method was found/used/set" [D:\a\firebase-cpp-sdk\firebase-cpp-sdk\BinaryCache\firebase\external\src\curl-build\lib\libcurl.vcxproj]

@darinf
Copy link
Author

darinf commented May 6, 2024

2024-05-06T00:46:09.3703288Z D:\a\firebase-cpp-sdk\firebase-cpp-sdk\BinaryCache\firebase\external\src\curl\lib\nonblock.c(89,1): error C1189: #error: "no non-blocking method was found/used/set" [D:\a\firebase-cpp-sdk\firebase-cpp-sdk\BinaryCache\firebase\external\src\curl-build\lib\libcurl.vcxproj]

Thanks. I've kicked off a build of compnerd/swift as well to see if it hits the same issue.

@compnerd
Copy link
Collaborator

compnerd commented May 6, 2024

Thanks. I've kicked off a build of compnerd/swift as well to see if it hits the same issue.

compnerd/swift is an unrelated experiment to implement a swift compiler pre-OSS. Did you mean the base repository? It might be more interesting to test this repository without your change (basically run the build at main), as that would tell us if the builds are currently working or not right?

@darinf
Copy link
Author

darinf commented May 6, 2024

Thanks. I've kicked off a build of compnerd/swift as well to see if it hits the same issue.

compnerd/swift is an unrelated experiment to implement a swift compiler pre-OSS. Did you mean the base repository? It might be more interesting to test this repository without your change (basically run the build at main), as that would tell us if the builds are currently working or not right?

Ah, OK. I just guessed at what branch to base my PR on. The instructions here: https://github.com/thebrowsercompany/swift-firebase?tab=readme-ov-file say to take the latest release from https://github.com/thebrowsercompany/firebase-cpp-sdk/tags, which appears to be a build off of compnerd/swift. Hmm.... what am I missing??

@compnerd
Copy link
Collaborator

compnerd commented May 6, 2024

Ah, OK. I just guessed at what branch to base my PR on. The instructions here: https://github.com/thebrowsercompany/swift-firebase?tab=readme-ov-file say to take the latest release from https://github.com/thebrowsercompany/firebase-cpp-sdk/tags, which appears to be a build off of compnerd/swift. Hmm.... what am I missing??

Oh! I see what you were saying - you were referring to the branch name not the repository name. Yes, that is the right branch to try to build at!

@darinf
Copy link
Author

darinf commented May 6, 2024

2024-05-06T00:46:09.3703288Z D:\a\firebase-cpp-sdk\firebase-cpp-sdk\BinaryCache\firebase\external\src\curl\lib\nonblock.c(89,1): error C1189: #error: "no non-blocking method was found/used/set" [D:\a\firebase-cpp-sdk\firebase-cpp-sdk\BinaryCache\firebase\external\src\curl-build\lib\libcurl.vcxproj]

Thanks. I've kicked off a build of compnerd/swift as well to see if it hits the same issue.

Looks like it also fails the firebase workflow:
https://github.com/thebrowsercompany/firebase-cpp-sdk/actions/runs/8972424774

Same issue:

2024-05-06T16:38:46.5437279Z D:\a\firebase-cpp-sdk\firebase-cpp-sdk\BinaryCache\firebase\external\src\curl\lib\nonblock.c(89,1): error C1189: #error:  "no non-blocking method was found/used/set" [D:\a\firebase-cpp-sdk\firebase-cpp-sdk\BinaryCache\firebase\external\src\curl-build\lib\libcurl.vcxproj]

@compnerd
Copy link
Collaborator

compnerd commented May 6, 2024

Seems like a recent regression then :(. Not sure what has happened here, but would need someone to investigate and fix before we can resume the builds (I'm currently tied up in a few things).

@compnerd
Copy link
Collaborator

compnerd commented May 6, 2024

2024-05-06T16:36:25.2980391Z -- Performing Curl Test HAVE_IOCTLSOCKET_CAMEL_FIONBIO
2024-05-06T16:36:25.8648467Z -- Performing Curl Test HAVE_IOCTLSOCKET_CAMEL_FIONBIO - Failed
2024-05-06T16:36:25.8650169Z -- Performing Curl Test HAVE_IOCTLSOCKET_FIONBIO
2024-05-06T16:36:26.4247802Z -- Performing Curl Test HAVE_IOCTLSOCKET_FIONBIO - Failed

I think that something is not right earlier in the environment resulting in the configure step mis-configuring CURL and thus causing the subsequent failure.

@darinf darinf force-pushed the darin/build-storage-and-functions branch from 40e719f to 5890332 Compare May 8, 2024 16:55
@darinf darinf force-pushed the darin/build-storage-and-functions branch from 5890332 to 630e908 Compare May 8, 2024 22:59
@darinf darinf merged commit e7be2e9 into compnerd/swift May 9, 2024
@darinf darinf deleted the darin/build-storage-and-functions branch May 9, 2024 04:15
@compnerd
Copy link
Collaborator

compnerd commented May 9, 2024

Do we know why upb.lib is no longer generated? This does worry me as there was a dependency on uprotobuf. Was it replaced with another library? I know that swift-firebase currently has a dependency on that particular library, did you test that with the updated nuget?

@darinf
Copy link
Author

darinf commented May 9, 2024

Do we know why upb.lib is no longer generated? This does worry me as there was a dependency on uprotobuf. Was it replaced with another library? I know that swift-firebase currently has a dependency on that particular library, did you test that with the updated nuget?

This remains a mystery. Haven't fully tested this. I will be back here if things don't work out properly.

@darinf
Copy link
Author

darinf commented May 9, 2024

Do we know why upb.lib is no longer generated? This does worry me as there was a dependency on uprotobuf. Was it replaced with another library? I know that swift-firebase currently has a dependency on that particular library, did you test that with the updated nuget?

This remains a mystery. Haven't fully tested this. I will be back here if things don't work out properly.

OK, yes... upb.lib appears to be needed:

PS C:\workspace\swift-firebase> swift build
Building for debugging...
lld-link: error: could not open 'upb.lib': no such file or directory
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[7/8] Linking C:\workspace\swift-firebase\.build\x86_64-unknown-windows-msvc\debug\FireBaseUI.exe

Will explore how to get it properly included into the nuget package.

github-actions bot pushed a commit that referenced this pull request May 16, 2024
Changes:
- Adds `FIREBASE_INCLUDE_{FUNCTIONS,STORAGE}` defines to the `cmake` build.
- Includes corresponding header files in the nuget package.
- Includes generated libs in the nuget package.
- Removes `upb.lib` which appears to no longer be generated.
bcny-fork-syncer bot pushed a commit that referenced this pull request May 25, 2024
Changes:
- Adds `FIREBASE_INCLUDE_{FUNCTIONS,STORAGE}` defines to the `cmake` build.
- Includes corresponding header files in the nuget package.
- Includes generated libs in the nuget package.
- Removes `upb.lib` which appears to no longer be generated.
bcny-fork-syncer bot pushed a commit that referenced this pull request May 31, 2024
Changes:
- Adds `FIREBASE_INCLUDE_{FUNCTIONS,STORAGE}` defines to the `cmake` build.
- Includes corresponding header files in the nuget package.
- Includes generated libs in the nuget package.
- Removes `upb.lib` which appears to no longer be generated.
bcny-fork-syncer bot pushed a commit that referenced this pull request Jun 1, 2024
Changes:
- Adds `FIREBASE_INCLUDE_{FUNCTIONS,STORAGE}` defines to the `cmake` build.
- Includes corresponding header files in the nuget package.
- Includes generated libs in the nuget package.
- Removes `upb.lib` which appears to no longer be generated.
bcny-fork-syncer bot pushed a commit that referenced this pull request Jun 7, 2024
Changes:
- Adds `FIREBASE_INCLUDE_{FUNCTIONS,STORAGE}` defines to the `cmake` build.
- Includes corresponding header files in the nuget package.
- Includes generated libs in the nuget package.
- Removes `upb.lib` which appears to no longer be generated.
bcny-fork-syncer bot pushed a commit that referenced this pull request Jul 16, 2024
Changes:
- Adds `FIREBASE_INCLUDE_{FUNCTIONS,STORAGE}` defines to the `cmake` build.
- Includes corresponding header files in the nuget package.
- Includes generated libs in the nuget package.
- Removes `upb.lib` which appears to no longer be generated.
bcny-fork-syncer bot pushed a commit that referenced this pull request Jul 18, 2024
Changes:
- Adds `FIREBASE_INCLUDE_{FUNCTIONS,STORAGE}` defines to the `cmake` build.
- Includes corresponding header files in the nuget package.
- Includes generated libs in the nuget package.
- Removes `upb.lib` which appears to no longer be generated.
bcny-fork-syncer bot pushed a commit that referenced this pull request Aug 3, 2024
Changes:
- Adds `FIREBASE_INCLUDE_{FUNCTIONS,STORAGE}` defines to the `cmake` build.
- Includes corresponding header files in the nuget package.
- Includes generated libs in the nuget package.
- Removes `upb.lib` which appears to no longer be generated.
bcny-fork-syncer bot pushed a commit that referenced this pull request Aug 6, 2024
Changes:
- Adds `FIREBASE_INCLUDE_{FUNCTIONS,STORAGE}` defines to the `cmake` build.
- Includes corresponding header files in the nuget package.
- Includes generated libs in the nuget package.
- Removes `upb.lib` which appears to no longer be generated.
bcny-fork-syncer bot pushed a commit that referenced this pull request Aug 21, 2024
Changes:
- Adds `FIREBASE_INCLUDE_{FUNCTIONS,STORAGE}` defines to the `cmake` build.
- Includes corresponding header files in the nuget package.
- Includes generated libs in the nuget package.
- Removes `upb.lib` which appears to no longer be generated.
bcny-fork-syncer bot pushed a commit that referenced this pull request Sep 7, 2024
Changes:
- Adds `FIREBASE_INCLUDE_{FUNCTIONS,STORAGE}` defines to the `cmake` build.
- Includes corresponding header files in the nuget package.
- Includes generated libs in the nuget package.
- Removes `upb.lib` which appears to no longer be generated.
bcny-fork-syncer bot pushed a commit that referenced this pull request Sep 11, 2024
Changes:
- Adds `FIREBASE_INCLUDE_{FUNCTIONS,STORAGE}` defines to the `cmake` build.
- Includes corresponding header files in the nuget package.
- Includes generated libs in the nuget package.
- Removes `upb.lib` which appears to no longer be generated.
bcny-fork-syncer bot pushed a commit that referenced this pull request Sep 12, 2024
Changes:
- Adds `FIREBASE_INCLUDE_{FUNCTIONS,STORAGE}` defines to the `cmake` build.
- Includes corresponding header files in the nuget package.
- Includes generated libs in the nuget package.
- Removes `upb.lib` which appears to no longer be generated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants