Skip to content

Conversation

@badboy
Copy link
Member

@badboy badboy commented Aug 8, 2023

I copied the script from https://github.com/mozilla/glean/blob/3c1c083238876dd6cc4e7577c66b73d309ab83d8/gradle-plugin/src/main/groovy/mozilla/telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy#L61

This now uses ~= for the glean-parser install, so we don't need to update glean.js for every glean_parser patch update.

@auto-assign auto-assign bot requested a review from rosahbruno August 8, 2023 09:41
@badboy badboy force-pushed the gleanparser-allow-any-patch-version branch from 26bc9f0 to 211faf7 Compare August 9, 2023 13:31
@badboy
Copy link
Member Author

badboy commented Aug 9, 2023

Probably needs a fix to bin/update-glean-parser-version.sh to only set the MAJOR.MINOR version correctly.

What to test:

  • Does it pick glean_parser 8.1.1 correctly on a clean source?
  • Manually install glean_parser 8.1.0 -- does it work?

@rosahbruno
Copy link
Contributor

rosahbruno commented Aug 9, 2023

@badboy

Probably needs a fix to bin/update-glean-parser-version.sh to only set the MAJOR.MINOR version correctly.

It looks like this command only updates two spots

  1. cli.ts - it will set whatever you pass in. If we run the script with version 8.0, it will set the value of GLEAN_PARSER_VERSION to 8.0
  2. qt/requirements.txt - we are not currently supporting any projects on QT and plan to likely decommission support, so likely not worth extra work there to deal with this

What to test:

* Does it pick glean_parser 8.1.1 correctly on a clean source?

* Manually install glean_parser 8.1.0 -- does it work?

I was able to git clean my repo and run and it picked up 8.1.1. I then manually pulled in 8.1.0 and it seemed to work correctly too.

Copy link
Contributor

@rosahbruno rosahbruno left a comment

Choose a reason for hiding this comment

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

Actually, before we merge. I think we should update these docs to specify that running this command should only use <MAJOR>.<MINOR> now.

https://github.com/mozilla/glean.js/blob/main/docs/guides/update_glean_parser.md?plain=1#L6

@badboy badboy force-pushed the gleanparser-allow-any-patch-version branch from 211faf7 to ee3cb05 Compare August 11, 2023 14:08
@badboy badboy requested a review from rosahbruno August 11, 2023 14:08
@rosahbruno rosahbruno merged commit 0a6e781 into main Aug 24, 2023
@rosahbruno rosahbruno deleted the gleanparser-allow-any-patch-version branch August 24, 2023 14:47
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