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

ICU-22324 Update Maven migration scripts #2661

Merged
merged 1 commit into from
Oct 5, 2023
Merged

ICU-22324 Update Maven migration scripts #2661

merged 1 commit into from
Oct 5, 2023

Conversation

echeran
Copy link
Contributor

@echeran echeran commented Oct 5, 2023

I updated the script to include files that moved directories since c7f227b. I didn't include files that were added or deleted since we can expect those changes to be handled by any version control system without getting confused.

Steps to reproduce

Here's effectively how I did my checking of the files across ICU 73.2 ("icu73") and the latest on upstream main ("icu74"):

git clone git@github.com:unicode-org/icu.git icu73
git clone git@github.com:unicode-org/icu.git icu74

pushd icu73
git checkout maint/maint-73
pushd icu4j
# Run toMaven.sh located in your development copy of ICU.
# Ex:
# ~/oss/icu/icu4j/maven-migration/toMaven.sh
popd
find icu4j -type f | egrep -v ".*.res$|.*.class$|.*.lst$|.*.cnv$" > icu4j-73-files.txt
popd

pushd icu74
find icu4j -type f | egrep -v ".*.res$|.*.class$|.*.lst$|.*.cnv$" > icu4j-74dev-files.txt
popd
Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22324
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@echeran echeran requested a review from mihnita October 5, 2023 07:22
markusicu
markusicu previously approved these changes Oct 5, 2023
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.

nice! thanks!

mihnita
mihnita reviewed Oct 5, 2023
View reviewed changes
icu4j/maven-migration/toMaven.sh Outdated
}

function moveOrRename() {
SRC=$1
Copy link
Contributor

@mihnita mihnita Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No point in defining SRC and DST if then mv uses $1 and $1

But even if we use $ everywhere, it would be nice to add a comment to (all) the functions with the parameters (so that one knows the order).
If I'm a Windows guy I might not know if mv is src dst or dst src

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

mihnita
mihnita reviewed Oct 5, 2023
View reviewed changes
icu4j/maven-migration/toMaven.sh Outdated
SRC=$1
DEST=$2

cp -R "$1" "$2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, no point in defining them.
BUT!
If you choose to define (and use) them, then you can do

local SRC=...
local DST=...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

mihnita
mihnita reviewed Oct 5, 2023
View reviewed changes
icu4j/maven-migration/toMaven.sh Outdated

cp -R "$1" "$2"
}

function safeMoveDir() {
export FOLDER_NAME=$1
Copy link
Contributor

Choose a reason for hiding this comment

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

s/export/local/
(I know that's what it was like, but meantime I learned better :-)
https://tldp.org/LDP/abs/html/localvar.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

mihnita
mihnita reviewed Oct 5, 2023
View reviewed changes
icu4j/maven-migration/toMaven.sh Outdated
DIR=$1
PATTERN=$2

find "$DIR" -type f -name "$PATTERN" -exec rm {} \;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: collapse multiple spaces
And comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

mihnita
mihnita reviewed Oct 5, 2023
View reviewed changes
icu4j/maven-migration/toMaven.sh Outdated
removeEclipseProjectFiles main/tests/packaging

# At this point this folder should be empty
# So remove if empty (-d) should work
rm -d main/core/src/test/java/com/ibm/icu/dev/test/serializable
}

function moveToolsFile() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: combine the 2 functions into one?
(moveFilesInTools and moveToolsFile)?
And comment.
I understand it is a matter of taste...
But I think it would be more readable if it's all in one, with comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mihnita
Copy link
Contributor

mihnita commented Oct 5, 2023

Please don't remove extract-data-files.sh (yet?)
It is very similar to icu4j/maven-migration/unpack_jars.sh, but a bit better (it "evolved" from that).
And it is used by the cldr data update (<icu_root>/docs/processes/cldr-icu.md)

Need to compare extract-data-files.sh and unpack_jars.sh to make sure nothing is missing, then move extract-data-files.sh into maven-migration and remove unpack_jars.sh
So that when one runs the migration script they end up with extract-data-files.sh in icu4j, the way main is today.

mihnita
mihnita reviewed Oct 5, 2023
View reviewed changes
Copy link
Contributor

@mihnita mihnita left a comment

Choose a reason for hiding this comment

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

Reviewed, thank you!
M.

@echeran
Copy link
Contributor Author

echeran commented Oct 5, 2023

Please don't remove extract-data-files.sh (yet?) It is very similar to icu4j/maven-migration/unpack_jars.sh, but a bit better (it "evolved" from that). And it is used by the cldr data update (<icu_root>/docs/processes/cldr-icu.md)

Need to compare extract-data-files.sh and unpack_jars.sh to make sure nothing is missing, then move extract-data-files.sh into maven-migration and remove unpack_jars.sh So that when one runs the migration script they end up with extract-data-files.sh in icu4j, the way main is today.

Done.

mihnita
mihnita approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@mihnita mihnita left a comment

Choose a reason for hiding this comment

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

Thank you!

@echeran echeran force-pushed the maven-migration-script-update branch from 9468f7b to d3e8d90 Compare October 5, 2023 19:51
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@echeran echeran merged commit deab8ee into unicode-org:main Oct 5, 2023
96 checks passed
@echeran echeran deleted the maven-migration-script-update branch October 5, 2023 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihnita mihnita mihnita approved these changes

@markusicu markusicu markusicu left review comments

Assignees

@markusicu markusicu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@echeran @mihnita @markusicu