-
-
Notifications
You must be signed in to change notification settings - Fork 269
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 --depwarn=yes by default in Pkg.test #1763
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c42f Thanks a lot for doing this! I've been wondering if I should create this PR.
src/Operations.jl
Outdated
@@ -1282,6 +1282,7 @@ function gen_test_code(testfile::String; | |||
--color=$(Base.have_color === nothing ? "auto" : Base.have_color ? "yes" : "no") | |||
--compiled-modules=$(Bool(Base.JLOptions().use_compiled_modules) ? "yes" : "no") | |||
--check-bounds=yes | |||
--depwarn=yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes Pkg.test
not to propagate --depwarn=error
. So I think we need to look at JLOptions()
and try not to "downgrade" depwarn
setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yes, on balance I think that's probably the most consistent with the other options. I added a tweak.
@@ -1282,6 +1282,7 @@ function gen_test_code(testfile::String; | |||
--color=$(Base.have_color === nothing ? "auto" : Base.have_color ? "yes" : "no") | |||
--compiled-modules=$(Bool(Base.JLOptions().use_compiled_modules) ? "yes" : "no") | |||
--check-bounds=yes | |||
--depwarn=$(Base.JLOptions().depwarn == 2 ? "error" : "yes") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I have started Julia explicitly with --depwarn=no
it would be nice to propagate that too, but maybe that info is not available as for other options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem like it is available. Guess you need to just pass it explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep there's not much we can do about this without considerably complicating Base.JLOptions()
with the ability to distinguish between default vs explicitly specified options.
Does this work in the default script of Travis CI? The |
JuliaLang/julia#35584 needs to be merged first |
Oh, I see. I had no doubt in my mind that this was already merged to |
If JuliaLang/julia#35362 is merged, adding
--depwarn=yes
by default will be important for continuity. See @tkf's comment JuliaLang/julia#35362 (comment) for more detail.(Even without merging that PR in Base, merging this PR should be harmless: it's currently the default in Julia and users can still disable it with
julia_args
.)