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.

Sign up for GitHub

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

Jump to bottom

fix(build): use cldr.version for cldr #345

Merged
merged 5 commits into from
Nov 29, 2022
Merged

fix(build): use cldr.version for cldr #345

merged 5 commits into from
Nov 29, 2022

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Oct 18, 2022 •
edited
Loading

  • github action should use the cldr.version for checkout
  • also bump the version of some actions

Fixes #316

@srl295 srl295 self-assigned this Oct 18, 2022
@srl295
Copy link
Member Author

srl295 commented Oct 18, 2022

yes i will propagate this to other workflows once its…working

@srl295 srl295 marked this pull request as ready for review October 18, 2022 21:24
@srl295 srl295 requested a review from echeran October 18, 2022 21:24
srl295
srl295 commented Oct 18, 2022
View reviewed changes
.github/workflows/push-jsp-on-tag.yml
- name: Set up JDK 11
uses: actions/setup-java@v1
with:
java-version: 11
- uses: actions/checkout@v2
Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it was checking out the repo twice!

Copy link
Member Author

Choose a reason for hiding this comment

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

probably a merge conflict

@srl295 srl295 force-pushed the srl295/issue316 branch 4 times, most recently from 0bc2502 to d97f06d Compare October 18, 2022 21:40
@srl295
Copy link
Member Author

srl295 commented Oct 18, 2022

so a major issue here is that github checkout, following security best practices, no longer handles the short hash to CLDR - actions/checkout#265

so the short hash such as 273d736f11 in 0.0.0-SNAPSHOT-273d736f11 is a challenge to checkout.

I'm going back to an older version of the action, but it will be a stopgap.

@srl295 srl295 force-pushed the srl295/issue316 branch 2 times, most recently from ed394ea to f0059e7 Compare October 18, 2022 21:52
@srl295
Copy link
Member Author

srl295 commented Oct 18, 2022

well this turned out to be kind of a mess… see if the caching helps at all.

@srl295
Copy link
Member Author

srl295 commented Oct 20, 2022

@echeran remainng build errors seem unrelated - ideas?

echeran
echeran reviewed Nov 18, 2022
View reviewed changes
.github/workflows/build-jsp.yml
with:
repository: unicode-org/cldr
path: cldr
ref: main
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, you can try using CLDR_REF here instead of main.

Copy link
Member Author

Choose a reason for hiding this comment

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

doesn't work because it's not a full hash.

@echeran
Copy link
Contributor

echeran commented Nov 18, 2022

@echeran remainng build errors seem unrelated - ideas?

I think the second mvn command for the "Generate Confusables" tool execution should have ...-am -pl ..., but I didn't add the -am part there. Can you try adding that in to the corresponding lines in the failing workflows?

That seems like the only thing that is failing, so hopefully that is the fix needed to get this PR ready to merge.

@markusicu
Copy link
Member

@srl295 please try @echeran 's suggestions. It would be great to get this PR done and in.

@srl295
Copy link
Member Author

srl295 commented Nov 29, 2022

@echeran remainng build errors seem unrelated - ideas?

I think the second mvn command for the "Generate Confusables" tool execution should have ...-am -pl ..., but I didn't add the -am part there. Can you try adding that in to the corresponding lines in the failing workflows?

That seems like the only thing that is failing, so hopefully that is the fix needed to get this PR ready to merge.

That was in the out-of-source, but the in-source was failing tests also.

srl295 and others added 3 commits November 29, 2022 11:11
@srl295 @echeran
fix(build): use cldr.version for cldr
afa390e 
- github action should use the cldr.version for checkout
- checkout all of CLDR (!) and then switch to the CLDR_REF

Fixes unicode-org#316

Co-authored-by: Elango Cheran <elango@unicode.org>
@srl295
fix: fix for out of source build
01a469f 
- Add -DfailIfNoTests=false to the retry-confusables call
@srl295
test: add a test for TestFmwkMinusMinus
049488e
@srl295
Copy link
Member Author

srl295 commented Nov 29, 2022

OK, spot the problem with in-source build:

$ mkdir cldr
$ ln -s cldr ..
$ cd ../cldr

… fixed

@srl295
fix(python): fix flake8 breakage
0e7ee1a 
- actions were failing, and a new install of flake8 locally broke for me as well

Ref: https://stackoverflow.com/a/74558566/185799
@srl295 srl295 requested a review from echeran November 29, 2022 18:04
srl295
srl295 commented Nov 29, 2022
View reviewed changes
.flake8
Comment on lines +8 to +13
ignore = E302,
# expected 2 blank lines, found 1
E701,
# multiple statements on one line
W504,
# line break after binary operator
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated deps based break

Broken:
https://github.com/unicode-org/unicodetools/actions/runs/3576355216/jobs/6014061731

Bad err message… debugged it to find the failing issue

came across

https://stackoverflow.com/questions/74558565/flake8-error-code-supplied-to-ignore-option-does-not-match-a-z1-30

from a few days ago

Copy link
Collaborator

Choose a reason for hiding this comment

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

This has been biting tons of Python devs who use flake8 for linting. Inline comments have always been "wrong", they're just enforcing it now :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

a better error message would have been nice…

And yes, it's open source and i haven't opened a PR yet.

@srl295
Copy link
Member Author

srl295 commented Nov 29, 2022

@echeran @markusicu in-source and python are passing again. out of source is getting much further. PTAL

Also rebased.

echeran
echeran approved these changes Nov 29, 2022
View reviewed changes
Copy link
Contributor

@echeran echeran left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

markusicu
markusicu approved these changes Nov 29, 2022
View reviewed changes
Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

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

looks... eminently plausible. thank you very much for getting this to work!

@srl295 srl295 merged commit 751cbfb into unicode-org:main Nov 29, 2022
@srl295 srl295 deleted the srl295/issue316 branch November 29, 2022 19:14
@srl295 srl295 mentioned this pull request Nov 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@echeran echeran

@markusicu markusicu

@josh-hadley josh-hadley

Assignees

@srl295 srl295

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

github action should use the cldr.version for checkout
4 participants
@srl295 @echeran @markusicu @josh-hadley