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

Erased mode in CI #3640

Open
wants to merge 12 commits into
base: leptos_0.8
Choose a base branch
from
Open

Conversation

zakstucke
Copy link
Contributor

@zakstucke zakstucke commented Feb 19, 2025

Add erase components as part of the test matrix, seems to work first try, although I am slightly surprised by that!

@zakstucke zakstucke marked this pull request as ready for review February 19, 2025 22:08
@benwis
Copy link
Contributor

benwis commented Feb 20, 2025

I also think it's working, but I am not the CI guru. Nice!

@benwis
Copy link
Contributor

benwis commented Feb 20, 2025

@sabify You're pretty familiar with CI I think, can you take a look at this? There's some weird Cache warnings stuff and clicking around in there I'm not even sure if it's running as we intend or if I'm blind. In some places it seems like it's failing to setup?

@sabify
Copy link
Contributor

sabify commented Feb 20, 2025

I don't see any issue with the workflow and the configuration flag has been set correctly in dev mode on. The only issue is that it is only running over member crates and not examples. I don't know if it is intended for that.

@benwis can you please elaborate more on "Cache warnings"? I don't see any warnings like that...

@sabify
Copy link
Contributor

sabify commented Feb 20, 2025

IMO, naming "dev_mode" is a bit misleading since you can be in dev mode without activating the erase_components cfg flag. Perhaps having its name be more clear here.

@benwis
Copy link
Contributor

benwis commented Feb 20, 2025

I don't see any issue with the workflow and the configuration flag has been set correctly in dev mode on. The only issue is that it is only running over member crates and not examples. I don't know if it is intended for that.

@benwis can you please elaborate more on "Cache warnings"? I don't see any warnings like that...

If I click into the checks and then click on a specific thing, it switches from passed to failed, and throws a warning. This page is an example of one: https://github.com/leptos-rs/leptos/actions/runs/13422224100

It could be possible that I'm jetlagged enough to not understand it, but it seems like things are wonky

@sabify
Copy link
Contributor

sabify commented Feb 20, 2025

You are right. I mentioned that the example CI left unmodified. @zakstucke You need to add the same flags from get-leptos-matrix.yml and ci.yml to the respective example CI configurations, ci-changed-examples.yml and ci-examples.yml.

@benwis
Copy link
Contributor

benwis commented Feb 20, 2025

Also adding a note here that we should figure out how to verify if it's in erase_components mode in the CI

@sabify
Copy link
Contributor

sabify commented Feb 20, 2025

IMO, naming "dev_mode" is a bit misleading since you can be in dev mode without activating the erase_components cfg flag. Perhaps having its name be more clear here.

I suggested that we may need to rename dev_mode.

@benwis
Copy link
Contributor

benwis commented Feb 20, 2025

There is a part of me that finds the idea of the CI just up and deciding it's done compiling and to start the tests is hilarious.

   Compiling wasm-bindgen v0.2.100
+ echo Times up, running tests
Times up, running tests
[cargo-make] INFO - Running Task: test-playwright
+ cd /home/runner/work/leptos/leptos/examples/error_boundary
+ BOLD=\e[1m
+ GREEN=\e[0;32m
+ RED=\e[0;31m
+ RESET=\e[0m
+ project_dir=/home/runner/work/leptos/leptos/examples/error_boundary
+ command -v pnpm
+ PLAYWRIGHT_CMD=pnpm
/home/runner/setup-pnpm/node_modules/.bin/pnpm
+ find . -name playwright.config.ts
+ dirname ./e2e/playwright.config.ts
+ pw_dir=./e2e
+ cd ./e2e
+ pnpm playwright test
   Compiling js-sys v0.3.77
   Compiling yoke-derive v0.7.5
   Compiling zerofrom v0.1.5

Probably worth looking at why the compile times changed here

@benwis
Copy link
Contributor

benwis commented Feb 21, 2025

This seems suboptimal:

[tasks.wait-server]
script = '''
for run in {1..12}; do
    echo "Waiting to ensure server is started..."
    sleep 10
done
echo "Times up, running tests"

@sabify
Copy link
Contributor

sabify commented Feb 22, 2025

This seems suboptimal:

[tasks.wait-server]
script = '''
for run in {1..12}; do
    echo "Waiting to ensure server is started..."
    sleep 10
done
echo "Times up, running tests"

I think a single 10s sleep will be enough...

@sabify
Copy link
Contributor

sabify commented Feb 23, 2025

@zakstucke Have you tried transparent route component (#[component(transparent)])? I guess it does work without needing to modify the macro... (At this point e36d7da; before modifying the macro)

Almost all route components in the failed CI examples were not transparent.

@zakstucke
Copy link
Contributor Author

zakstucke commented Feb 23, 2025

@sabify this wouldn't work because the parent component, the one calling the nested routing component, is expecting AnyNestedRoute in erased mode, and can't handle the generic impl return type. It's very specific to that split routing case, and the way I've done it requires no breaking change/interop between erased and normal mode. Couldn't find anything better than a proc macro hack, so I think it's fine for this case.

@sabify
Copy link
Contributor

sabify commented Feb 23, 2025

@zakstucke I just replaced #[component] with #[component(transparent)] over suspense_tests' InstrumentedRoutes() and it worked.

@zakstucke
Copy link
Contributor Author

zakstucke commented Feb 23, 2025

@sabify I think you must've checked in non-erased mode, at e36d7da, setting InstrumentedRoutes to #[component(transparent)] and running RUSTFLAGS="--cfg erase_components" cargo test fails with method cannot be called on "impl MatchNestedRoutes + Clone" due to unsatisfied trait bounds.

With current HEAD both erased and non-erased work.

@sabify
Copy link
Contributor

sabify commented Feb 23, 2025

@zakstucke This is what I've done and I still don't get any error:

git clone https://github.com/leptos-rs/leptos
cd leptos/examples/suspense_tests
cargo clean
sed -i '205s/.*/#[component(transparent)]/' src/instrumented.rs
RUSTFLAGS="--cfg=erase_components" cargo test --features=ssr
RUSTFLAGS="--cfg=erase_components" cargo test --features=hydrate
# OR
RUSTFLAGS="--cfg=erase_components" cargo build --features=ssr
RUSTFLAGS="--cfg=erase_components" cargo build --features=hydrate
# OR
RUSTFLAGS="--cfg=erase_components" cargo leptos build

@zakstucke
Copy link
Contributor Author

zakstucke commented Feb 23, 2025

This is on the 0.8 branch, where routing itself is erased, 0.7 wouldn't have this problem as routing was not being erased

@sabify
Copy link
Contributor

sabify commented Feb 23, 2025

Ooops! Thanks for the clarification and your hard work on this feature.

@benwis
Copy link
Contributor

benwis commented Feb 23, 2025

Glad to see you two collaborating. Looks like we've only got one failure left to fix!

@zakstucke
Copy link
Contributor Author

The last ones a bit of a headscratcher. It's not an issue with erased mode, but a rust limitation where if you cross compile to a different target e.g. wasm, the proc macro doesn't respect RUSTFLAGS, so it's in a situation where leptos_macro is compiling in non-erased, and the rest of the crates in erased, leading to the error.

The good news is it means testing erased mode for wasm/frontend after this is fixed will probably be faster, bad news is I have absolutely no idea how to solve it 😅 .

rust-lang/cargo#4423
https://users.rust-lang.org/t/rustflags-ignored-when-cross-compiling-a-proc-macro-crate/92537

@sabify
Copy link
Contributor

sabify commented Feb 23, 2025

How about adding CARGO_TARGET_WASM32_UNKNOWN_UNKNOWN_RUSTFLAGS: ${{ inputs.erased_mode && '--cfg erase_components' || '' }} besides RUSTFLAGS in .github/workflows/run-cargo-make-task.yml?

@zakstucke
Copy link
Contributor Author

zakstucke commented Feb 23, 2025

It's actually the other way round, it's the host platform that doesn't respect them (e.g. mac or linux), because rust deems the RUSTFLAGS shouldn't apply to build.rs or proc macros, only the final build artefact. As far as I can see there's absolutely no workaround that works, it's pretty ridiculous.

I think I've found a solution though, an internal __internal__erase_components feature for leptos_macro that get's automatically enabled by the leptos crate. This doesn't have the same drawbacks we initially didn't want to use a feature for erasure for, because this is internal and auto enabled.

@zakstucke
Copy link
Contributor Author

The fix for leptos_macro properly erasing has unearthed an issue with a few checks in the suspense test annoyingly, I've ran out of time to work on this for now. 

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.

3 participants