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

feat: allow building with the system Lua #969

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Kladki
Copy link
Contributor

@Kladki Kladki commented Apr 28, 2024

As per #943 (comment)

I tried to use default-features = false in the deps of yazi-fm and yazi-cli and have vendored-lua to be default in the dependencies, but when I did that it would just build with vendored lua every time. This is the only method that I could get to actually work, sorry if it's not ideal. :(

@sxyazi
Copy link
Owner

sxyazi commented Apr 28, 2024

Is it possible to still keep each crate's default = [ "vendored-lua" ], but prevent them from being referenced in e.g. yazi-fm and yazi-cli using something like yazi-dds = { path = "../yazi-dds", version = "0.2.5", default-features = false }?

@Kladki
Copy link
Contributor Author

Kladki commented Apr 28, 2024

I can try, but that would likely require setting default-features = false in every Cargo.toml as some of the other yazi dependencies depend on other ones which use the ones which depend on mlua.

@Kladki Kladki force-pushed the optional-vendored-lua branch from 122c5f8 to 5b58e92 Compare April 28, 2024 15:25
@Kladki
Copy link
Contributor Author

Kladki commented Apr 28, 2024

Done. Nvm forgot to add the defaults again No apparently they were already there, ready!

@Kladki Kladki force-pushed the optional-vendored-lua branch from 5b58e92 to fd0e456 Compare April 28, 2024 15:27
@Kladki Kladki force-pushed the optional-vendored-lua branch from fd0e456 to 045d13a Compare April 28, 2024 15:27
yazi-dds = { path = "../yazi-dds", version = "0.2.5" }
yazi-plugin = { path = "../yazi-plugin", version = "0.2.5" }
yazi-proxy = { path = "../yazi-proxy", version = "0.2.5" }
yazi-dds = { path = "../yazi-dds", version = "0.2.5", default-features = false }
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need to add default-features = false to yazi-core? yazi-core is not the entry point of the program, is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because that dependency would otherwise expect to have those features, and we cannot override that without making enabling that feature not the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cargo tree seems unreliable for these tests, so I just eza -l target/release and compare the sizes. Without this there is no size difference between the default and --no-default-features, but with this there is a difference.

Copy link
Owner

Choose a reason for hiding this comment

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

What command did you use for building?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cargo build with and without --release and with and without --no-default-features

@sxyazi sxyazi force-pushed the main branch 5 times, most recently from 6c0a482 to f5598eb Compare July 13, 2024 06:22
@sxyazi sxyazi force-pushed the main branch 2 times, most recently from 4eec20b to ed6ae00 Compare July 16, 2024 10:50
@sxyazi sxyazi force-pushed the main branch 6 times, most recently from 33d04f9 to 3fdbbfe Compare August 2, 2024 09:43
@sxyazi sxyazi force-pushed the main branch 2 times, most recently from 72571df to 9483798 Compare September 7, 2024 05:52
@sxyazi sxyazi force-pushed the main branch 4 times, most recently from 6b59636 to 6aced05 Compare October 25, 2024 00:31
@sxyazi sxyazi force-pushed the main branch 4 times, most recently from 9ddaba4 to ef1a31a Compare October 26, 2024 09:09
@sxyazi sxyazi force-pushed the main branch 12 times, most recently from f2e33da to c65bdb3 Compare November 14, 2024 05:08
@sxyazi sxyazi force-pushed the main branch 10 times, most recently from 2b70f8d to 5e48df5 Compare November 26, 2024 09:11
@sxyazi sxyazi force-pushed the main branch 2 times, most recently from 43473db to d72f903 Compare November 30, 2024 13:26
@sxyazi sxyazi force-pushed the main branch 2 times, most recently from 1394fb4 to 62ac224 Compare December 9, 2024 02:28
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.

2 participants