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.

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

Bootstrap flutter_webrtc + rust + libwebrtc toolchain (#4) #5

Merged
merged 62 commits into from
Nov 17, 2021

Conversation

logist322
Copy link

@logist322 logist322 commented Nov 3, 2021

Synopsis

Этот PR добавляет необходимый toolchain для разработки flutter-webrtc под WIndows.

Checklist

  • Created PR:
    • In draft mode
    • Name contains Draft: prefix
    • Name contains issue reference
    • Has k:: labels applied
    • Has assignee
  • Documentation is updated (if required)
  • Tests are updated (if required)
  • Changes conform code style
  • CHANGELOG entry is added (if required)
  • FCM (final commit message) is posted
    • and approved
  • Review is completed and changes are approved
  • Before merge:
    • Milestone is set
    • PR's name and description are correct and up-to-date
    • Draft: prefix is removed
    • All temporary labels are removed

@logist322 logist322 added enhancement Improvement of existing features or bugfix k::toolchain Applies to changes of project toolchain labels Nov 3, 2021
@logist322 logist322 self-assigned this Nov 3, 2021
@logist322 logist322 changed the title Draft: Bootstrap flutter_webrtc + rust + libwebrtc toolchain (#4) Draft: Bootstrap flutter_webrtc + rust + libwebrtc toolchain (!4) Nov 3, 2021
@logist322 logist322 changed the title Draft: Bootstrap flutter_webrtc + rust + libwebrtc toolchain (!4) Draft: Bootstrap flutter_webrtc + rust + libwebrtc toolchain (#4) Nov 3, 2021
@logist322
Copy link
Author

logist322 commented Nov 5, 2021

FCM

Bootstrap toolchain for desktop development under Windows (#5, #4)

- add `libwebrtc-sys` and `flutter-webrtc-native` Rust crates
- add `Makefile`
- bootstrap CI pipeline

@logist322 logist322 removed the review label Nov 5, 2021
@logist322 logist322 removed the request for review from alexlapa November 5, 2021 10:04
Copy link
Member

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@logist322 one thing should be done more wisely.

Makefile Outdated
# make deps.thirdparty
deps.thirdparty:
mkdir -p temp && \
curl -L -o temp/libwebrtc-win-x64.tar.gz $(LIBWEBRTC_URL)/libwebrtc-win-x64.tar.gz && \
Copy link
Member

Choose a reason for hiding this comment

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

This should be implementent in build.rs of libwertc-sys crate and the link controlled via env var. Keeping it in Makefile only complicates stuff.

fn main() {
let path = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap());
#[rustfmt::skip]
let libwebrtc_url = "https://github.com/instrumentisto/libwebrtc-bin/releases/download/97.4692.0.0-r0";

Choose a reason for hiding this comment

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

@logist322 ,

This should be implementent in build.rs of libwertc-sys crate and the link controlled via env var.

and the link controlled via env var

env var

@@ -0,0 +1 @@
target/

Choose a reason for hiding this comment

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

@logist322 ,

Newline добавьте в конец. По хорошему, IDE'шка должна сама это уметь.

@tyranron ,

Вы этот .gitignore выкосили, что логично, так как крейт в вокрспейсе, и /target лежит не тут. Но тут немного другая история, мы в build.rs вот такое делаем:

    cbindgen::generate_with_config(&crate_dir, config)
        .unwrap()
        .write_to_file(format!("target/{}.hpp", package_name));

Ложить хедеры в корневой target не получается, так как переменная CARGO_BUILD_TARGET_DIR не прокидывается, по неизвестной причине, а хардкодить ../../target не хочется, но можем, если Вы за такое решение.

Copy link
Member

Choose a reason for hiding this comment

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

@tyranron it's better to investigate what's the problem with CARGO_BUILD_TARGET_DIR. It should be working OK.

Copy link

@alexlapa alexlapa Nov 16, 2021

Choose a reason for hiding this comment

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

@tyranron ,

Make cargo export CARGO_TARGET_DIR to build.rs scripts

Build scripts are intended to have a very scoped output (just in their OUT_DIR), and they're not currently really intended to be used as installation scripts for other sorts of files. In that sense this change was never intended for build scripts.

rust-lang/cargo#7325 (comment)

А OUT_DIR это вот: /flutter-webrtc/target/debug/build/flutter-webrtc-native-621c22406cba9de9/out. Бахаю ../../target. От нее и пойдем вверх до /target.

@logist322 logist322 requested review from alexlapa and removed request for alexlapa November 16, 2021 09:51
@tyranron tyranron removed the review label Nov 16, 2021
@tyranron
Copy link
Member

tyranron commented Nov 16, 2021

@logist322 don't use review label on GitHub. Asking regular GitHub review is enough.

And also don't use Draft: prefix. Using GitHub draft PR status is enough.

@alexlapa alexlapa changed the title Draft: Bootstrap flutter_webrtc + rust + libwebrtc toolchain (#4) Bootstrap flutter_webrtc + rust + libwebrtc toolchain (#4) Nov 17, 2021
@logist322 logist322 merged commit 1b5c95e into windows-rework Nov 17, 2021
@logist322 logist322 deleted the bootstrap branch November 17, 2021 11:42
@tyranron tyranron added platform::windows Specific to Windows platform feature New feature or request and removed enhancement Improvement of existing features or bugfix labels Dec 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request k::toolchain Applies to changes of project toolchain platform::windows Specific to Windows platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants