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

Add Cargo Packager support #18

Conversation

naman-crabnebula
Copy link
Contributor

Enable the bundling of the Verso Browser using Cargo Packager.

  • Now, we can bundle the Verso browser using Cargo Packager
  • cargo fmt is run to eliminate formatting issues.
  • Placeholder icon is used
  • Already tested on Linux packages ( AppImage )

@wusyong
Copy link
Member

wusyong commented Mar 27, 2024

I mentioned we could probably embed the resource files instead. I'm not fond of two different behaviors between debug and release.
Is there any problem when using something like include_bytes?

@naman-crabnebula
Copy link
Contributor Author

I mentioned we could probably embed the resource files instead

We can deal that separately I guess

I'm not fond of two different behaviors between debug and release.

Well, it is not different at all. We are just using resource resolver to resolve the resources in the build environment.

But if we want to embed the files, we can do that later.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
Cargo.toml Outdated
@@ -37,6 +52,8 @@ crossbeam-channel = "0.5"
getopts = "0.2.17"
surfman = { version = "0.9", features = ["chains", "sm-angle", "sm-angle-default", "sm-x11", "sm-raw-window-handle"] }
winit = { version = "0.29", features = ["rwh_05"] }
# Cargo Packager
cargo-packager-resource-resolver = { version = "0.1", features = ["auto-detect-format"] }
Copy link
Member

Choose a reason for hiding this comment

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

I read the documentation of cargo-packager-resource-resolver and it seems it requires cargo packager.
I don't want any release builds being restrict to cargo packager only. Otherwise, other users won't be able to build their own release profiles. Is it possible we just access somewhere in release build? I saw there's resources tag mentioned above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we must use any sort of packager out there to create bundles. So, it is just better to use cargo packager instead of 3 or more separate crates.

Otherwise, other users won't be able to build their own release profiles

Why? Cargo Packager is an open source project, and any user can just install it and build their own release profile.

I don't see any alternative as well as any reason against using cargo packager to be honest.

Copy link
Member

Choose a reason for hiding this comment

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

Servo uses bundle manual commands in their mach scripts. There could be people want to build dmg all by themselves. And cargo package hasn't supported Flatpak yet which is a key format in verso's feature request issue. We should contain cargo packager metadata in Cargo.toml only.

I believe we don't need cargo packager to establish a CN cloud workflow for now. Just try to upload the binary as an artifact should be enough. We can worry about package format later.

Choose a reason for hiding this comment

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

cargo-packager-resource-resolver does rely on cargo-packager's CLI. we could move it behind a feature flag to support custom build systems

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines +4 to +6
push:
branches:
- "*"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
push:
branches:
- "*"
pull_request:
branches:
- main

@@ -37,6 +54,8 @@ crossbeam-channel = "0.5"
getopts = "0.2.17"
surfman = { version = "0.9", features = ["chains", "sm-angle", "sm-angle-default", "sm-x11", "sm-raw-window-handle"] }
winit = { version = "0.29", features = ["rwh_05"] }
# Cargo Packager
cargo-packager-resource-resolver = { version = "0.1", features = ["auto-detect-format"], optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

I updated a commit that can embed all resource files into binary. I believe we don't need to handle file path then.

@wusyong wusyong mentioned this pull request Apr 16, 2024
@denjell-crabnebula
Copy link
Member

What ended up happening here?

@wusyong
Copy link
Member

wusyong commented Jul 3, 2024

There were several build fails. And I would like to figure out #21 and #26.
I think it would be easier if we can focus on one platform only instead of bundling every platform.

@wusyong
Copy link
Member

wusyong commented Jul 11, 2024

I think we can revisit this feature again. All we need to worry is bundling the resource directory together.

@pewsheen pewsheen mentioned this pull request Jul 18, 2024
5 tasks
@pewsheen
Copy link
Collaborator

I created an issue to follow the status: #85

Since the PR's base is pretty old, I created another branch for this and added @naman-crabnebula as a co-author.

@wusyong wusyong closed this Jul 18, 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.

5 participants