feat: let Provider use ArtifactCache

[ralbertazzi]

Pull Request Check List

Follow-up of #7621. Closes #2415

@radoering most of the PR is about initializing the ArtifactCache and passing it around the codebase whenever it's needed. Let me know if you prefer a different approach. For instance I saw usages of Config.create() around the codebase (such as in CachedRepository) that would simplify the changes a lot, as one would simply need to create an ArtifactCache right inside get_package_from_url. On the other side I like the DI approach as it allows for better modularity and testability, as the cost of more verbose code. I'll not fight this one though, I'll gladly go for the option you recommend ;)

• Added tests for changed code. -> I'll do it as soon as we agree on the best way to refactor the code
• [ ] Updated documentation for changed code. Don't think it's needed

[radoering]

For instance I saw usages of Config.create() around the codebase (such as in CachedRepository) that would simplify the changes a lot

... and probably introduce bugs. This only considers the global config and neglect the local config. (I'm aware that there are such bugs in the code base.) Here you can see what's necessary to create a config that considers global and local settings

poetry/src/poetry/factory.py

Lines 62 to 71 in 71b3d6e

	# Loading global configuration
	config = Config.create()
	# Loading local configuration
	local_config_file = TOMLFile(base_poetry.file.parent / "poetry.toml")
	if local_config_file.exists():
	if io.is_debug():
	io.write_line(f"Loading configuration file {local_config_file.path}")
	config.merge(local_config_file.read())

[ralbertazzi]

@radoering forgive me for summoning you here, I'm just unsure whether your comment is actually part of the review - suggesting that I should go for a simpler refactor that uses Config.create() but that also considers the local config - or if it was just an agreement comment and I should actually wait for your proper review

[radoering]

Rather agreement. I just wanted to prevent you from doing a refactoring and calling Config.create() in various places. Even if the local config is considered, I don't think it's a good idea to read the config files several times. So nothing to do for now.

[ralbertazzi]

@radoering while we finalize the form of this PR, would it be possible to include #7621 in the next Poetry release? It should be a harmless change but it would allow me to write a very easy plugin using the ArtifactCache and start using it ASAP in my organization. I'd make sure the plugin will be compatible with these changes ;)

[radoering]

#7621 will be included in the next minor release since it has already been merged into master. However, the next release is not yet imminent so this PR might make it in, too.

[ralbertazzi]

Thanks for the great contribution, I definitely like the changes!
I also added a few tests to cover DirectOrigin.

Although I'm eager to see this PR merged, I just thought that Poetry supports a --no-cache argument, while this PR will always cache URL dependencies regardless of the parameter value. To be fair the documentation mentions it more as a debugging parameter, which is the reason why I'd still be in favour of merging it and see if anybody complains through an issue (I'm sure this change would get more praises than complaints).

As per the conflicts, I'd kindly leave that to you since you have been the author of the latest PRs merged and this way I'm sure I'll not break anything :D

[radoering]

Although I'm eager to see this PR merged, I just thought that Poetry supports a --no-cache argument, while this PR will always cache URL dependencies regardless of the parameter value.

I also thought about it. However, as far as I know --no-cache as well as the poetry cache command only handle <cache-dir>/cache and ignore <cache-dir>/artifacts. That's why I think we should not take --no-cache into account here. Improving the documentation, let alone extending the command/option to consider the artifact cache is something for another PR.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.