Use user scope for eval() invocation in @artifact_str
#47121
Conversation
stdlib/Artifacts/src/Artifacts.jl
Outdated
| find_artifacts_toml(srcfile) | ||
| else | ||
| eval(artifacts_toml_path) | ||
| Core.eval(__module__, artifacts_toml_path) |
There was a problem hiding this comment.
Instead of eval use getfield? But that may just be personal taste.
There was a problem hiding this comment.
How would you use getfield? In this case, artifacts_toml_path could be an expression, rather than a quoted string.
There was a problem hiding this comment.
I would prefer not using a runtime/order dependent lookup here. eval inside a macro is for me a code-smell. Either the data is a literal and can be relied upon, or it is a runtime value and we should defer it's evaluation to inside the quote block.
|
Couldn't we just require this to always be a literal string? Consumers could still do |
That was the original idea but Elliot added the eval in (0f309bc), likely due to some use case that I had not thought about. |
|
Can we revert that? This seems quite fishy. |
DilumAluthge
force-pushed
the
sf/fix_artifacts
branch
from
d63002c to
5bcc6d2
Compare
staticfloat
force-pushed
the
sf/fix_artifacts
branch
from
5bcc6d2 to
64c2426
Compare
My assertion in the previous attempt to fix this issue was incorrect:
> We define a simpler match as one that has fewer tags overall.
> As these candidate matches have already been filtered to match the
> given platform, the only other tags that exist are ones that are in
> addition to the tags declared by the platform. Hence, selecting the
> minimum in number of tags is equivalent to selecting the closest match.
This is demonstrably false, by my own test case:
```
platforms = Dict(
Platform("x86_64", "linux") => "bad",
Platform("x86_64", "linux"; sanitize="memory") => "good",
)
select_platform(platforms, Platform("x86_64", "linux"; sanitize="memory")) == "good"
```
In this case, because there exists a candidate that is _more general_
than the provided platform type, the shortest match is no longer the
best match.
This PR performs a more rigorous matching that works more reliably in
all cases.
staticfloat
force-pushed
the
sf/fix_artifacts
branch
from
64c2426 to
e1de57f
Compare
This causes the eval to be invoked in user scope, rather than in the
Artifactsmodule itself.X-ref: #46755 (review)