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

Add prefix to extension #90

Closed
wants to merge 3 commits into from

Conversation

devmotion
Copy link
Contributor

Extensions with non-unique name cause collisions and hence break e.g. compilation of sysimages. This PR adds a prefix to the extension to work around that issue. See JuliaMath/ChangesOfVariables.jl#13 for a longer discussion.

@jw3126
Copy link
Member

jw3126 commented Feb 24, 2023

Thanks a lot! Can you also prefix the other extensions?

@aplavin
Copy link
Member

aplavin commented Feb 24, 2023

Unfortunate that it's needed for now. I guess it's temporary? See JuliaLang/Pkg.jl#3380 (comment).
Results in long and redundant names - eg in precompilation output:

  ✓ LogExpFunctions → LogExpFunctionsInverseFunctionsExt
  ✓ LogExpFunctions → LogExpFunctionsChainRulesCoreExt
  ✓ LogDensityProblemsAD → LogDensityProblemsADEnzymeExt
  ✓ LogDensityProblemsAD → LogDensityProblemsADForwardDiffBenchmarkToolsExt
  ✓ LogDensityProblemsAD → LogDensityProblemsADForwardDiffExt
  ✓ LogDensityProblemsAD → LogDensityProblemsADReverseDiffExt
  ✓ LogDensityProblemsAD → LogDensityProblemsADTrackerExt
  ✓ LogDensityProblemsAD → LogDensityProblemsADZygoteExt

and in Project.toml.
Some official scheme of extension naming would be nice.

@devmotion
Copy link
Contributor Author

Well, I assume this is desired even if the name collision issues are fixed, see e.g. JuliaMath/ChangesOfVariables.jl#13 (comment).

@devmotion
Copy link
Contributor Author

Some official scheme of extension naming would be nice.

I guess it's already established inofficially, most packages that I'm aware of use these prefixes (e.g., SciML, SpecialFunctions, and LogExpFunctions).

@aplavin
Copy link
Member

aplavin commented Feb 24, 2023

Oh, there are LOTS of packages that don't do prefixing - just search github as https://github.com/search?q=%5Bweakdeps%5D+lang%3ATOML&type=code.

@devmotion
Copy link
Contributor Author

Github doesn't show any results, is the link you posted correct? In any case, I'm sure there are packages which don't use prefixes but a major part of the ecosystem, including core packages, already does. And I guess it's rather these packages which form inofficial standards.

@aplavin
Copy link
Member

aplavin commented Feb 24, 2023

Github doesn't show any results, is the link you posted correct?

Just clicked again - seems to work. It's search for [weakdeps] across all TOML-type files.

I'm not against this PR anyway.
It's just unlikely to work at scale, duplicate names will inevitably appear.
And this prefixing leads to redundancy in messages, that's not helpful.

@aplavin
Copy link
Member

aplavin commented Feb 24, 2023

a major part of the ecosystem, including core packages, already does

I think it's mostly SciML and related, btw. Eg LoopVectorization, Plots, MPI, PGFPlotsX, PkgCacheInspector, StaticArrayInterface don't use prefixes - and these include core julia developers. This is just from the first search page :)

@devmotion
Copy link
Contributor Author

Just clicked again - seems to work. It's search for [weakdeps] across all TOML-type files.

That's what I assumed but it doesn't work for me:
image

And this prefixing leads to redundancy in messages, that's not helpful.

I wonder if that might be (partly) a complaint about the standard Pkg messages rather than the names of the extensions. I imagine that if there is an error in one of the extension modules then standard logging/error messages might only point you to ChainRulesCoreExt, and it might not be immediately clear to which parent package the extension belongs.

Generally, I'm not surprised that quite a few packages don't use prefixes yet. Many packages switched to prefixes only very recently (e.g., SpecialFunctions a few days ago), now that it became apparent that there are benefits.

@aplavin
Copy link
Member

aplavin commented Feb 24, 2023

that might be (partly) a complaint about the standard Pkg messages rather than the names of the extensions

If anything, it's natural to show both the parent package name and extension name in any messages - be it Pkg, errors, or whatever.

Copy link
Member

@jw3126 jw3126 left a comment

Choose a reason for hiding this comment

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

LGTM. We should add the Accessors prefix for now, if it is not needed in future anymore, we can always drop it again. Thanks @devmotion

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