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

feat(mirror): add Jihulab as optional git repo #249

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

leeebo
Copy link
Contributor

@leeebo leeebo commented Jun 17, 2024

This PR aims to add another git mirror for users to choose

Jihulab is a Chinese localization of Gitlab public repository,

Espressif maintained an official mirror there https://jihulab.com/esp-mirror/espressif/esp-idf,

Which has the same submodule relative paths as the Github repo.

@leeebo leeebo force-pushed the feat/add_jihulab_git_mirror branch from f52a0b6 to 86120c9 Compare June 17, 2024 03:35
Copy link
Collaborator

@jakub-kocka jakub-kocka left a comment

Choose a reason for hiding this comment

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

Thanks @leeebo for the PR proposal, I have left a few comments. Otherwise, it looks good to me.

But I have noticed that the Jihulab option is checked out by default, I think we should update this for the default version of the Installer to be not checked.

src/InnoSetup/Summary.iss Outdated Show resolved Hide resolved
src/InnoSetup/Languages/IdfToolsSetup_sk-SK.isl Outdated Show resolved Hide resolved
src/InnoSetup/Languages/IdfToolsSetup_cs-CZ.isl Outdated Show resolved Hide resolved
@jakub-kocka
Copy link
Collaborator

One more thing, maybe we should have Jihulab and Gitee dependent on each other, and when one option is set the other one will not be possible to set.

@leeebo
Copy link
Contributor Author

leeebo commented Jun 21, 2024

One more thing, maybe we should have Jihulab and Gitee dependent on each other, and when one option is set the other one will not be possible to set.

Yes, make sense. I want to do that, but I don't know the syntax, that's why I write else if here

Will fix it

@jakub-kocka
Copy link
Collaborator

One more thing, maybe we should have Jihulab and Gitee dependent on each other, and when one option is set the other one will not be possible to set.

Yes, make sense. I want to do that, but I don't know the syntax, that's why I write else if here

Will fix it

This should be only for summary text so it is OK to put there just if.
TBH, I am unsure about the syntax as well, but if you need some help, I can dig a bit more into how to implement this.

@leeebo leeebo force-pushed the feat/add_jihulab_git_mirror branch 4 times, most recently from dd4d633 to 77e1532 Compare June 26, 2024 10:30
@leeebo leeebo force-pushed the feat/add_jihulab_git_mirror branch from 77e1532 to 9ec0a84 Compare June 27, 2024 02:00
@leeebo
Copy link
Contributor Author

leeebo commented Jun 27, 2024

@jakub-kocka Now users can exclusively check or uncheck the Git Mirror like below:

English Language:
image

Chinese Language:
07fb808481b20b619d8f684a446a661

I have tested it on my Windows11 PC, it works as expected, the log as bellow:
Setup Log 2024-06-27 #003.txt

However, it should be noted that since the submodule of the submodule may specify the URL from github directly (like below, release/v5.3, cmock's submodules are from github), even if the user chooses Git mirror, these submodules still download from github. As suggested by @wujiangang, we may need to run a command like git config - global url.https://jihulab.com/esp-mirror.insteadOf https://github.com to fix it, then unset it after installation. @jakub-kocka @georgik what's your opinion?

image

@jakub-kocka
Copy link
Collaborator

@leeebo Nice work! Thank you for the improvements.

I agree with changing the git URL. If it may wait a bit longer we can fix this in the scope of the task with git clone fix (internal tracker IDF-10228 - it is planned for Q3) so this PR can be eventually merged.

@leeebo
Copy link
Contributor Author

leeebo commented Jun 28, 2024

@jakub-kocka Comment has been added in the jira, let's wait for your fix

@jakub-kocka
Copy link
Collaborator

@jakub-kocka Comment has been added in the jira, let's wait for your fix

@leeebo, thank you.

So this PR is ready to merge, right?

@dobairoland
Copy link
Collaborator

LGTM as far as I can tell.

@dobairoland
Copy link
Collaborator

@leeebo Do you think this mirror would be useful to mention at https://docs.espressif.com/projects/esp-idf/en/latest/esp32/get-started/linux-macos-setup.html#step-2-get-esp-idf? We have later on that page Github mirror for downloadable tools but no alternative mentioned to cloning from Github.

@jakub-kocka
Copy link
Collaborator

Thank you @dobairoland for the review.
I am merging this and will release the latest IDF version offline installer and online installer.

@jakub-kocka jakub-kocka merged commit e2769c3 into espressif:main Jul 1, 2024
@leeebo
Copy link
Contributor Author

leeebo commented Jul 2, 2024

@dobairoland Thanks for your suggestions, I think finally we should mention the mirrors at https://docs.espressif.com/projects/esp-idf/en/latest/esp32/get-started/linux-macos-setup.html#step-2-get-esp-idf, we currently need an additional step to change the submodule's submodule URL from github to jihulab.

How about adding a new script like tools/set-submodules-to-jihulab.sh ? in esp-idf

@dobairoland
Copy link
Collaborator

we currently need an additional step to change the submodule's submodule URL from github to jihulab.

No, I don't think that is necessary. We use relative paths here: https://github.com/espressif/esp-idf/blob/master/.gitmodules. That means that the submodules URLs will point to the same page domain the root repository was cloned from. The same way it works for our Github & Gitlab. I don't see why it wouldn't work for other mirrors as well.

@leeebo
Copy link
Contributor Author

leeebo commented Jul 2, 2024

@dobairoland For the ESP-IDF's submodules it's OK, but for submodules's submodule, eg. CMock has submodule vendor/unity and vendor/unity, which using specified URL instead of relative paths. https://github.com/ThrowTheSwitch/CMock/blob/eeecc49ce8af123cf8ad40efdb9673e37b56230f/.gitmodules

@dobairoland
Copy link
Collaborator

Ok, I see. I wouldn't add a script to ESP-IDF which is not even needed directly by it. But I'm sure we can have such helper script in another repository.

Depending on the number of such submodules we can consider to submit PRs into them.

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