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

Apply coursier default config files (repositories and mirrors config) #2886

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

jilen
Copy link
Contributor

@jilen jilen commented Nov 21, 2023

Fix #2312

Copied from
https://github.com/coursier/coursier/blob/main/modules/coursier/shared/src/main/scala/coursier/Resolve.scala#L61

It is possible to use Resolve().finalRepositories.future() to avoid the implementation detail.

But that requires Await and ExecutionContext, I'm not sure whether worth doing it.

@lefou
Copy link
Member

lefou commented Nov 23, 2023

I'd favor just calling the configuration logic from coursier. Copied code will get out of sync eventually. Await is standard Scala, so it should be fine to use it. It's easy to identify as boilerplate when reviewing the code.

@jilen jilen force-pushed the apply_coursier_config_files branch from 5b88cc5 to dd0344b Compare November 24, 2023 01:34
@jilen
Copy link
Contributor Author

jilen commented Nov 24, 2023

@lefou Now it is implemented using finalRepositories .

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Yeah, that's better.
We can import the execution context more local, e.g. just in the task.
Beside that, it looks good to me.

How did you test this? Would it be possible to add a small integration test under integration.feature or example.misc?

@jilen
Copy link
Contributor Author

jilen commented Nov 24, 2023

Yeah, that's better. We can import the execution context more local, e.g. just in the task. Beside that, it looks good to me.

How did you test this? Would it be possible to add a small integration test under integration.feature or example.misc?

I test it by remove repo1.maven.org in the coursier cache directory. And it won't appear again.
I'll try to add some test

@lefou
Copy link
Member

lefou commented Nov 24, 2023

In an integration test, we could use a different empty temporary cache location and could either assert, that Mill fails as expected, or fills that repo as expected.

@lefou lefou marked this pull request as draft November 27, 2023 10:23
@jilen jilen force-pushed the apply_coursier_config_files branch from 8302c3d to 9da85e5 Compare December 1, 2023 02:00
@jilen jilen marked this pull request as ready for review December 1, 2023 02:29
@jilen
Copy link
Contributor Author

jilen commented Dec 1, 2023

@lefou I've add a test suite by setting the courser.mirrors system property.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for adding a test!

@lefou lefou merged commit 452bf68 into com-lihaoyi:main Dec 1, 2023
37 checks passed
@lefou lefou added this to the after 0.11.6 milestone Dec 1, 2023
@lefou
Copy link
Member

lefou commented Dec 1, 2023

Thank you for this contribution, @jilen !

@lefou
Copy link
Member

lefou commented Dec 1, 2023

This is available in 0.11.6-10-452bf6.

@lefou
Copy link
Member

lefou commented Dec 4, 2023

@jilen It would be really cool, if you could review our documentation regarding repository configuration. We should mention all potential ways to customize it. IIUC, we now also support some coursiers-specific config files.

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.

can not config repository mirror via ~/.config/coursier/mirror.properties
2 participants