Skip to content

ci: Fix wonky FFI builds#501

Closed
wiredsis wants to merge 1 commit into
livekit:mainfrom
wiredsis:ffi-ci-fix
Closed

ci: Fix wonky FFI builds#501
wiredsis wants to merge 1 commit into
livekit:mainfrom
wiredsis:ffi-ci-fix

Conversation

@wiredsis
Copy link
Copy Markdown

@wiredsis wiredsis commented Dec 2, 2024

The FFI builds have been failing due to the WebRTC license file not existing. Since that file is only generated when compiling to Android as a target, this PR ensures the file exists in that location, and swaps double quotation marks for single ones.

The FFI builds have been failing due to the WebRTC license file not
existing. Since that file is only generated when compiling to Android as
a target, this PR ensures the file exists in that location, and swaps
double quotation marks for single ones.
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Dec 2, 2024

CLA assistant check
All committers have signed the CLA.

@tommady
Copy link
Copy Markdown
Contributor

tommady commented Dec 2, 2024

Could you take a look at this PR? I believe it might be the root cause, but I’m curious why this .gitignore was added to exclude the LICENSE file.

cat LICENSE >> TEMP_LICENSE.md
echo "```" >> TEMP_LICENSE.md
echo '```' >> TEMP_LICENSE.md
touch livekit-ffi/WEBRTC_LICENSE.md
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Regarding my comment about the .gitignore excluding the LICENSE file, simply creating an empty LICENSE file doesn’t seem like a good solution.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fair enough. I chose this route since, as seen here, the file is only generated when the target is an Android device. Maybe a better alternative would be running this job when an Android target matches.

@theomonnom
Copy link
Copy Markdown
Member

This file should be generated for each platform:

cp "$OUTPUT_DIR/LICENSE.md" "$ARTIFACTS_DIR"

@theomonnom
Copy link
Copy Markdown
Member

I think the issue is coming from this PR #495

@tommady
Copy link
Copy Markdown
Contributor

tommady commented Dec 2, 2024

Ah, I see now—thank you for taking the time to explain. Just to confirm, is the CI issue related to correctly addressing the $ARTIFACTS_DIR?

If so, I stand by my review: creating an empty LICENSE file doesn’t effectively address the core issue. 🙇🏻‍♂️

By the way, regarding the PR you mentioned (#495), I want to clarify that I didn’t make any changes to the build_linux.sh file. In that PR, I explicitly stated that my intent was to resolve the issues with the cargo doc build and the CI tests.

@theomonnom
Copy link
Copy Markdown
Member

#518

@theomonnom theomonnom closed this Dec 12, 2024
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.

4 participants