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

RFC: Add a simple feature checking mechanism. #48764

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Feb 23, 2023

This PR adds a simple mechanism to check if a build of Julia has certain features:

julia> Base.has_feature("features")
true

julia> Base.has_feature("whatever")
false

The purpose of this function is to use this to drive conditional code in packages. Currently, we use:

  • VERSION checks, e.g. if VERSION >= v"1.10.0-DEV.649", which doesn't tell me much if I read this code
  • checking for specific functionality, e.g., if isdefined(Base, :get_extension)

The other problem with version checks is that they need the Julia PR to be merged before we can adapt packages. For example, in #48611 PkgEval revealed quite some breakage. I'd like to fix packages without having to merge the (re-landed version of) that PR, but there isn't any easy way to check for the changed behavior without looking deeply into compiler internals, which is ugly. In fact, I think the isdefined(Base, :get_extension) is ugly too, and a call to Base.has_feature("extensions") would be much easier on the eyes.

Currently, the set of features is hard-coded, just as a starting point for this PR. I considered automating its generation during build, e.g. making it possible to check for PR numbers, but AFAIK that would require use of the GitHub API during build which isn't great.

@maleadt maleadt added the kind:speculative Whether the change will be implemented is speculative label Feb 23, 2023
@test Base.has_feature("features")
f() = Base.has_feature("features")
code = sprint(code_warntype, f, Tuple{})
@test occursin("(\"features\")::Core.Const(true)", code)
Copy link
Member Author

Choose a reason for hiding this comment

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

There probably is a better way to check for const-inferred results.

Copy link
Member

Choose a reason for hiding this comment

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

symbols for example would be a lot better here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? That wouldn't change the outcome of the call being folded away (i.e., it'd still contain a const-inferred call to Base.has_features, now just passing :features instead of "features"). Or do you mean returning a symbol instead of a const boolean?

Copy link
Member

Choose a reason for hiding this comment

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

strings generally have worse effects than symbols since they're technically mutable

@quinnj
Copy link
Member

quinnj commented Feb 24, 2023

I love this idea and I'm surprised we didn't add this ages ago. I guess you have to remember to add the feature flag, but we already have a "checklist" of sorts we go thru when adding new stuff (tests, docs, news item, etc.).

@BioTurboNick
Copy link
Contributor

I like this in principle, but I wonder if the use-case is a bit narrow and may in practice be too much overhead for what it adds?

Just thinking about how small some new features could be, or how they could change in dev builds; and then once released, they won't be removed. It would make more sense to me if there were components sometimes present and sometimes not, e.g. due to running on different platforms.

If the main issue is how packages interact with PRs to Julia in dev builds, maybe there's a better solution that just addresses that specifically? Perhaps dev builds could have a list of merged PR numbers (that gets reset on release?), and you could check for the presence of that PR number? Just brainstorming here.

@maleadt
Copy link
Member Author

maleadt commented Feb 24, 2023

Perhaps dev builds could have a list of merged PR numbers (that gets reset on release?), and you could check for the presence of that PR number?

Sure, that would be fine, but how do you automate that in a reliable way? It also needs to work for builds of an unmerged PR, and I don't think there's a way to determine the PR number in such a situation without using the GitHub API. It also needs to work outside of GitHub, so we can't rely on there being a merge commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:speculative Whether the change will be implemented is speculative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants