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

Set Random and Test to be weak deps #152

Merged
merged 1 commit into from
Oct 14, 2023
Merged

Conversation

jakobnissen
Copy link
Collaborator

The test-related functionality is still there, but requires loading Random and Test.

Note that THIS MIGHT BE A BREAKING CHANGE, since users could call these functions without loading Test or Random previously.
However, I think we just need to make this breaking change. There is no way around it, and as we begin moving more stuff out of the sysimage, it becomes increasingly annoying that TranscodingStreams has these needless deps.

@jakobnissen jakobnissen requested a review from mkitti October 7, 2023 11:44
@mkitti
Copy link
Member

mkitti commented Oct 7, 2023

Do we actually need these to be package extensions?

@jakobnissen
Copy link
Collaborator Author

You mean, whether we can just delete them functions instead? I think they're useful for downstream dependents, and it's more breaking to completely remove them.

Copy link
Member

@sjkelly sjkelly left a comment

Choose a reason for hiding this comment

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

Seems like a reasonable middle ground between deletion and hard-dep! LGTM!

@mkitti
Copy link
Member

mkitti commented Oct 10, 2023

Overall, I think this is headed in the right direction. Let's consider it a breaking change. I bumped the minor version to reflect this.

@mkitti
Copy link
Member

mkitti commented Oct 10, 2023

For the version change, it looks like will need to update a few packages' compat for CI to pass:
https://github.com/JuliaIO/CodecBase.jl/blob/master/Project.toml

@jakobnissen
Copy link
Collaborator Author

Can we just merge this anyway? None of these downstream packages will actually be affected by this change, since their compat will prevent them from installing version 0.10 (which is indeed why this CI fails).
I think we should also fix CI for this package such that it doesn't auto-break for new breaking releases, but that's a separate issue.

@jakobnissen
Copy link
Collaborator Author

Actually, let's remove the downstream tests from TS's tests, and add it to this repo's CI instead.

@jakobnissen jakobnissen force-pushed the weak_test branch 2 times, most recently from 67c21de to fe2b857 Compare October 13, 2023 18:52
The test-related functionality is still there, but requires loading Random
and Test.
@jakobnissen
Copy link
Collaborator Author

@mkitti I believe this is ready to merge now. Should I push the button?

@mkitti mkitti merged commit 337f418 into JuliaIO:master Oct 14, 2023
17 of 18 checks passed
@jakobnissen jakobnissen deleted the weak_test branch October 14, 2023 05:37
@KristofferC
Copy link
Member

KristofferC commented Jan 16, 2024

Looking into the PkgEval failures here JuliaLang/julia#52841 (comment), the reason for those is that this packages say that it needs both Random and Test to load the extension, but then never end up loading Random.

This used to previously work due to a bug / circumstance in that Random is in the sysimage but this behavior will be fixed with JuliaLang/julia#52841.

What can be done here and the other packages that fail are:

  • Import Random in the main package.
  • Add Random as a test dep and import it in runtests.jl.

cc @mkitti, @jakobnissen

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.

4 participants