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

Parsing error on latest release #162

Closed
max-sixty opened this issue Aug 12, 2023 · 9 comments · Fixed by #164
Closed

Parsing error on latest release #162

max-sixty opened this issue Aug 12, 2023 · 9 comments · Fixed by #164

Comments

@max-sixty
Copy link

FYI we're getting ~150 annotations on our jobs in the past few hours, because of warnings around parsing Cargo.toml.

https://github.com/PRQL/prql/actions/runs/5843523532

Our Cargo.toml is fairly standard, I had thought? Though not impossible it's doing something non-standard... https://github.com/PRQL/prql/blob/f84022c3e2703cc99b7237ba24fc18480f0b6744/Cargo.toml

@NobodyXu
Copy link
Contributor

Tl; dr, this is the warning on CI:

Warning: Error parsing Cargo.toml manifest, fallback to caching entire file: SyntaxError: Expected "=", [ \t] or [A-Za-z0-9_\-] but "." found.

@NobodyXu
Copy link
Contributor

NobodyXu commented Aug 12, 2023

This is caused by the toml parser, it's probably not good enough to handle every case.

rust-cache uses toml from nodejs, do you have any recommendation on a better one?

@max-sixty
Copy link
Author

rust-cache uses toml from nodejs, do you have any recommendation on a better one?

I'd like to be helpful but unfortunately I know very little about the nodejs ecosystem...

@NobodyXu
Copy link
Contributor

NobodyXu commented Aug 13, 2023

rust-cache uses toml from nodejs, do you have any recommendation on a better one?

I'd like to be helpful but unfortunately I know very little about the nodejs ecosystem...

Thanks, luckily rust-cache does fallback to caching the entire Cargo.toml, so the cache would still work although it might invalidate the cache more often in workspace.

@max-sixty
Copy link
Author

Yeah, the main issue is that we get 150 warnings, which somewhat invalidates the warnings we do want to see...

@NobodyXu
Copy link
Contributor

Hmmm maybe adding an option to suppress these warnings?

@max-sixty
Copy link
Author

Yeah, or reverting and just using a hash of the file, depending on how widespread this issue is...

@NobodyXu
Copy link
Contributor

Yeah, or reverting and just using a hash of the file, depending on how widespread this issue is...

Well, the warning is actually added in #156 , it can be reverted without removing the parsing part.

@NobodyXu
Copy link
Contributor

I will try replacing the toml dependency with one specified in #163 (comment)

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 a pull request may close this issue.

2 participants