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

Ensure @artifact is skipped on non-windows platforms #443

Merged
merged 1 commit into from Sep 24, 2020

Conversation

staticfloat
Copy link
Sponsor Member

Because newer @artifact macros do more work at compile-time, we need to use a compile-time if statement here to avoid running the macro at all on non-windows systems.

Fixes #442

Because newer `@artifact` macros do more work at compile-time, we need to use a compile-time `if` statement here to avoid running the macro at all on non-windows systems.

Fixes #442
@fredrikekre
Copy link
Member

Thanks. I suspected this was the problem. What work does it do during macro expansion, and why?

@staticfloat
Copy link
Sponsor Member Author

It loads, parses and extracts the needed hash all at compile time, so as to save on time and space when loading the .ji file in the future. Here's the commit, with the associated message: JuliaLang/julia@2b2ee1f#diff-ccd5c99b75897c5269d89a687f8b0dc6

It only does that work at compile-time when the artifact name is a constant string; if you give it the result of a function call, or a variable or something like that, it will have to do the work at runtime.

@tkf
Copy link
Member

tkf commented Sep 21, 2020

If you need a fix like this PR, doesn't it mean JuliaLang/julia#37320 is backward incompatible? Maybe Pkg.Artifacts should export the old run-time -only version of @artifact_str? Also, I wonder if the compile-time constancy has a negative implication for relocatability: JuliaLang/julia#37681

@staticfloat
Copy link
Sponsor Member Author

I think this consequence is a small enough backwards compatibility that we don’t need to worry about it too much. It’s an easy enough fix.

@tkf
Copy link
Member

tkf commented Sep 21, 2020

I understand "minor change" is a thing in Julia. But I'd like to note that https://semver.org does not says it's OK to introduce breaking change if it is easy to fix. Furthermore, it is trivial to make Pkg.Artifacts.@artifact_str backward-compatible because it is a different namespace than Artifacts.

(Technically, I guess it is possible to argue that this is not breaking since Pkg.Artifacts.@artifact_str API documentation does not specify when the key lookup happens. But given that most of Base/stdlib APIs are under-specified, I don't think it's OK to use this as an argument.)

Anyway, it's probably not a good idea for me to rant about SemVer compliance of julia here. You might have considered all the engineering trade-offs before breaking compatibility. I hope it's not going to introduce confusion in public and private projects using artifacts.

@DilumAluthge
Copy link
Member

Could we get a new release of PackageCompiler that includes this fix?

@KristofferC
Copy link
Sponsor Member

Yep, done.

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.

Artifacts stdlib broke artifact"..." macro ?
5 participants