Feat/wesl-toml#185
Conversation
|
|
||
| let pattern_str = pattern_path.to_string_lossy(); | ||
| let glob_iter = | ||
| glob::glob(&pattern_str).map_err(|e| ScanTomlError::InvalidGlob(pattern.clone(), e))?; |
There was a problem hiding this comment.
We cannot actually use the glob library for finding all relevant wesl files. We have two requirements
- One shouldn't visit the same file twice
- Excluded directories should be completely skipped
The first requirement is not straightforward to pull off, since we start out with an array of globs. Here's an example of a nasty user input ["./shaders/**/*", "./shaders/../shaders/*", "./shaders/foo/*"]
One way of correctly doing it is with the algorithm from here webgpu-tools/wesl-spec#172
And since we cannot really use the glob library, we shouldn't be including it just to validate the globs.
There was a problem hiding this comment.
Excluded directories are skipped in the end, see isExcluded, though doing it after the fact is inefficient.
Visiting a file twice during scanning nasty user input doesn't seem tragic. The current implementation deduplicates, so multiple paths shouldn't be reported, though I see there's a quick fix needed there for the pathological case.
I can also try a fix with the more custom scanning approach. That would be a bit more efficient at runtime, and give us a base for whatever we decide on spec #172. I suppose it'll be a fair bit more code than relying on glob to do most the work, so we could make that a separate PR depending on what y'all prefer.
There was a problem hiding this comment.
I think addressed the other suggestions but perhaps best to defer this one? That'll keep this largish PR from growing even larger. Scanning efficiency improvements and an implementation for spec#172 would go well together.
|
added |
|
@mighdoll I've combined your representation layout with the stricter one I used in branch wesl-toml. I delegate parsing to intermediate types with to/from conversion functions for the parts which don't fit easily serde's data model (in particular dependencies). Didn't touch the scanning logic which looks good. The PR is ready to review |
mighdoll
left a comment
mighdoll
left a comment
There was a problem hiding this comment.
@k2d222 the changes look good me, thx for the lessons. They pass the tests upstream on https://github.com/mighdoll/lygia/tree/feat/cargo too.
within lygia, import lygia::math; works now
New PkgBuilder::scan_toml() builds packages from config.
add test for pathological user include patterns add a comment about why base_dir is prepended
more efficient / less error prone for npm users
…eserialization to proxy types
|
what's next for this PR? I hope I didn't do something wrong with the gh wrangling.. is there more to discuss or implement? |
|
looks good to me! I added one fix for #149: sub-dependencies are renamed |
|
awesome and I will dbl check the draft lygia cargo branch too |
| #[derive(Clone, Debug, Serialize, Deserialize)] | ||
| pub struct WeslToml { | ||
| /// Package configuration. | ||
| #[serde(flatten)] |
There was a problem hiding this comment.
package fields in wesl.toml are flattened.
package fields are flattened into the wesl.toml root
webgpu-tools#185 includes a test case for a pathological wesl.toml config with overlapping globs previously, we deduplicated for that by using canonical fs paths, following symlinks to absolute paths. But pnpm uses symlinks inside node_modules/, so absolute paths might not match user exclude paths in wesl.toml. This broke in lygia when I was using a link: dependency for development. In this commit we switch to using lexical path normalization for deduplication, processing '.' and '..' in paths. That way user exclude paths match. And if a user wants to intentionally duplicate a file or path with a symlink, they can now do that too.
* fix for wesl.toml glob dedup w/o following symlinks #185 includes a test case for a pathological wesl.toml config with overlapping globs previously, we deduplicated for that by using canonical fs paths, following symlinks to absolute paths. But pnpm uses symlinks inside node_modules/, so absolute paths might not match user exclude paths in wesl.toml. This broke in lygia when I was using a link: dependency for development. In this commit we switch to using lexical path normalization for deduplication, processing '.' and '..' in paths. That way user exclude paths match. And if a user wants to intentionally duplicate a file or path with a symlink, they can now do that too. * Replace glob expansion with directory walk to support nested wesl.toml * fmt * use Path::absolute on wesl.toml root path intead of manually stripping ./ * simplify error report during stack walk
3 participants
PkgBuilder::scan_toml() builds packages from config.
fix for #149
and fix build warning