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

yew-macro: remove transitive dependency on syn 1 #3752

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Alextopher
Copy link

@Alextopher Alextopher commented Oct 21, 2024

Description

I've been enjoying yew and wanted to find a place I could make small contributions. I'm happy to put in some effort to remove duplicate dependencies.

There's a rustsec release out on proc-macro-error, the issue being the crate is unmaintained. Potential replacements include proc-macro-error2 which seems to be API compatible. proc-macro-error2 also removes some needless build scripts improving compilation times.

Overall, replacing proc-macro-error with proc-macro-error2 leads to an 8% reduction in (single threaded) compilation of yew.

cargo build --release --timings -j1 results below.

Before
image

After
image

Checklist

The existing test suite passes, I don't believe there is a test I could write for this PR.

Before

$ cargo tree -d | grep syn
syn v1.0.109
syn v2.0.79

After

$ cargo tree -d | grep syn

ranile
ranile previously approved these changes Oct 21, 2024
Copy link
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

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

Thanks for making the change

Copy link
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

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

I regenerated the test outputs to fix the CI error and found this issue

Comment on lines 1 to 6
warning: The tag 'tExTAreA' is not matching its normalized form 'textarea'. If you want to keep this form, change this to a dynamic tag `@{"tExTAreA"}`.
--> tests/html_lints/fail.rs:17:10
|
17 | <tExTAreA />
| ^^^^^^^^

Copy link
Member

Choose a reason for hiding this comment

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

For some reason, the update to proc-macro-error2 crate made it so that this lint is triggered. Can you please investigate and see what's going on?

To re-generate the files, you can run the test command with TRYBUILD=overwrite

Copy link
Author

Choose a reason for hiding this comment

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

I haven't been able to reproduce the issue on my machine yet. I'll try to work through this.

Copy link
Member

Choose a reason for hiding this comment

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

You need to use the same command as CI:

RUSTFLAGS='--cfg nightly_yew --cfg yew_lints' cargo +nightly test -p yew-macro test_html_lints

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I'm going to go ahead and bisect the proc-macro-errors2 project.

@Alextopher
Copy link
Author

I think this will fix the nightly test, but will trigger stable failures.

@Alextopher
Copy link
Author

Alextopher commented Oct 21, 2024

Using yew-nightly as a cargo feature means using the --all-features flag on stable causes build failures. I imagine that could be a pretty big cost, and might be why you use cargo attributes instead.

Maybe it's possible to remove usage of nightly features from proc-macro-errors2 but that was out of scope of my original goal.

I'm sorry, I'm not sure how else to work around this.

@Alextopher Alextopher requested a review from ranile October 21, 2024 16:30
@Alextopher
Copy link
Author

Further research - removal of those build scripts I was speaking of (in favor of using a feature nightly) means we can't just drop-in replace yew's usage of proc-macro-error with proc-macro-error2.

Maybe we could use a build script to enable that feature using the version-check crate. Is there some way to conditionally enable a feature-flag based on the usage of a nightly compiler?

@ranile
Copy link
Member

ranile commented Dec 13, 2024

I attempted a fix in 37452a5 but it seems cargo does not allow this: rust-lang/cargo#8170

We cannot switch to using feature flag for nightly (see #2754). I wanted to use buildscript to enable the feature but that is not possible either. I raised GnomedDev/proc-macro-error-2#7 to allow using cfg here, which can be used from build.rs

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