Skip to content

Fix parsing some YAMLs due to outdated libyaml#7112

Merged
deivid-rodriguez merged 1 commit intomainfrom
deivid-rodriguez/libyaml-0.2.5
Apr 20, 2023
Merged

Fix parsing some YAMLs due to outdated libyaml#7112
deivid-rodriguez merged 1 commit intomainfrom
deivid-rodriguez/libyaml-0.2.5

Conversation

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

While working on PNPM, I was going crazy because Dependabot could not parse the following YAML file:

https://github.com/ice-lab/icestark/blob/ec2d44244a2b4ebd7af0e0ff0a10f528b33b8a64/pnpm-lock.yaml

However, it would be parsed just fine on my dev machine.

On my dev machine:

$ ruby -rpsych -e 'Psych.load_file("tmp/ice-lab/icestark/pnpm-lock.yaml")'
# ok

On the development container:

$ ruby -rpsych -e 'Psych.load_file("tmp/ice-lab/icestark/pnpm-lock.yaml")'
/usr/local/lib/ruby/gems/3.1.0/gems/psych-4.0.4/lib/psych.rb:455:in `parse': (tmp/ice-lab/icestark/pnpm-lock.yaml): did not find expected ',' or '}' while parsing a flow mapping at line 1308 column 17 (Psych::SyntaxError)
	from /usr/local/lib/ruby/gems/3.1.0/gems/psych-4.0.4/lib/psych.rb:455:in `parse_stream'
	from /usr/local/lib/ruby/gems/3.1.0/gems/psych-4.0.4/lib/psych.rb:399:in `parse'
	from /usr/local/lib/ruby/gems/3.1.0/gems/psych-4.0.4/lib/psych.rb:323:in `safe_load'
	from /usr/local/lib/ruby/gems/3.1.0/gems/psych-4.0.4/lib/psych.rb:369:in `load'
	from /usr/local/lib/ruby/gems/3.1.0/gems/psych-4.0.4/lib/psych.rb:671:in `block in load_file'
	from /usr/local/lib/ruby/gems/3.1.0/gems/psych-4.0.4/lib/psych.rb:670:in `open'
	from /usr/local/lib/ruby/gems/3.1.0/gems/psych-4.0.4/lib/psych.rb:670:in `load_file'
	from -e:1:in `<main>'

After investigating, turns out I was using a newer version of libyaml locally, which must've corrected some bugs.

Unfortunately, updating the underlying libyaml is painful, but I think it can be done, so this is my take at it.

An alternative approach would be to avoid parsing YAML. For my purposes is doable, since I just wanted to detect the version used by the PNPM lockfile and that can be figured out pretty reliably without parsing YAML. But I was worried that this could be biting us in other places so I'm trying to upgrade libyaml.

@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner April 19, 2023 09:19
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/libyaml-0.2.5 branch from 28f39cf to 4069b91 Compare April 19, 2023 09:31
Copy link
Copy Markdown
Contributor

@brrygrdn brrygrdn left a comment

Choose a reason for hiding this comment

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

This upgrade makes sense to me!

Regarding the test failures, the Updater spec failure looks like we might have a new flake potentially introduced by #7075 - it's worth rebasing/merging main to see if it clears up just in case but we might need to go look at that.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

I hope it will be good after #7113 🤞.

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/libyaml-0.2.5 branch from 4069b91 to 58249a7 Compare April 19, 2023 11:12
@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

Well, that didn't do the trick. Hopefully it's fine after #7114.

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/libyaml-0.2.5 branch 3 times, most recently from 5e11cae to fe1c1c9 Compare April 19, 2023 16:36
Copy link
Copy Markdown
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Seems fine to me.

It looks like Focal (which we're on now) and Jammy (which we'll be moving to in #5030) both pull in 0.2.2 but starting in Kinetic (22.10) libyaml-dev defaults to 0.2.5: https://packages.ubuntu.com/search?suite=default&section=all&arch=any&keywords=libyaml-dev&searchon=names

Copy link
Copy Markdown
Member

@jeffwidman jeffwidman Apr 19, 2023

Choose a reason for hiding this comment

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

Should this change happen regardless of the libyaml bump? Or does somehow nuking the entire ~/.bundle dir help with docker image caching more than nuking ~/.bundle/cache?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's because I need to configure Bundler globally to use the downloaded sources and the previous rm -rf ~/.bundle would wipe out the configuration, which is also needed when installing gems from omnibus/.

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/libyaml-0.2.5 branch from 9dcc65e to 97a5c98 Compare April 19, 2023 19:26
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/libyaml-0.2.5 branch from 97a5c98 to 56b5383 Compare April 20, 2023 16:45
While working on PNPM, I was going crazy because Dependabot could not
parse the following YAML file:

https://github.com/ice-lab/icestark/blob/ec2d44244a2b4ebd7af0e0ff0a10f528b33b8a64/pnpm-lock.yaml

However, it would be parsed just fine on my dev machine.

After investigating, turns out I was using a newer version of libyaml
locally, which must've corrected some bugs.

Unfortunately, updating the underlying libyaml is painful, but I think
it can be done, so this is my take at it.

An alternative approach would be to avoid parsing YAML. For my purposes
is doable, since I just wanted to detect the version used by the PNPM
lockfile and that can be figured out pretty reliably without parsing
YAML. But I was worried that this could be biting us in other places so
I'm trying to upgrade libyaml.
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/libyaml-0.2.5 branch from 56b5383 to df47240 Compare April 20, 2023 18:10
@deivid-rodriguez deivid-rodriguez enabled auto-merge (squash) April 20, 2023 18:10
@deivid-rodriguez deivid-rodriguez merged commit 570898a into main Apr 20, 2023
@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/libyaml-0.2.5 branch April 20, 2023 19:26
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