search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.38k stars 470 forks source link

a PR claiming a fix must validate it #38192

Closed dimpase closed 4 months ago

dimpase commented 4 months ago

#38163 claims to fix #38159, but does not provide a test showing this.

Thus, if #38163 is closed, it will trigger closing #38159. Thus the claim should be removed, or a followup PR or issue opened.

dimpase commented 4 months ago

Another problem with https://github.com/sagemath/sage/pull/38163 is that it has made an optional package tdlib dependent upon an experimental package sagemath_environment. This is not how it is supposed to work, an optional package can only depend on optional and standard packages. @dcoudert - please retract your positive review.

@vbraun - please refrain from merging https://github.com/sagemath/sage/pull/38163 in its present form.

EDIT - sorry, I take this back. #38163 is OK in this sense - the breaking change was added much earlier, in #35661

dimpase commented 4 months ago

And the blocker #38190 also points at #38163 being responsible for breaking the build in some cases.

saraedum commented 4 months ago

Another problem with #38163 is that it has made an optional package tdlib dependent upon an experimental package sagemath_environment. This is not how it is supposed to work, an optional package can only depend on optional and standard packages.

Could you explain where that dependency is? It's not in dependencies at least.

dimpase commented 4 months ago

with #38163, it is in the build dependencies:

cat build/pkgs/sagemath_tdlib/dependencies
tdlib cysignals boost_cropped | $(PYTHON_TOOLCHAIN) sage_setup sagemath_environment cython pkgconfig $(PYTHON)
saraedum commented 4 months ago

I don't see that in the changeset of the PR. What am I doing wrong?

dimpase commented 4 months ago

perhaps it was already there, before this PR (afk now). Do a git blame after the checkout?

dimpase commented 4 months ago

then this PR by itself is innocent (apologies as I didn't check this), but the blocker still stands.

dimpase commented 4 months ago

This can be closed as soon as there is a test certifying that https://github.com/sagemath/sage/issues/38159 got fixed.

S17A05 commented 4 months ago

Since #38163 does not claim to fix #38159 anymore and @dimpase set the PR back to "positive review", I am closing this issue - please reopen if I misunderstood.

dimpase commented 4 months ago

@S17A05 :

https://github.com/sagemath/sage/pull/38163 claims to fix https://github.com/sagemath/sage/issues/38159, but does not provide a test showing this.

Thus, if https://github.com/sagemath/sage/pull/38163 is closed, it will trigger closing https://github.com/sagemath/sage/issues/38159. Thus the claim should be removed, or a followup PR or issue opened.

Is there a followup?

S17A05 commented 4 months ago

@S17A05 :

38163 claims to fix #38159, but does not provide a test showing this.

Thus, if #38163 is closed, it will trigger closing #38159. Thus the claim should be removed, or a followup PR or issue opened.

Is there a followup?

Not that I know of; however, the claim was removed - is that not what you were asking for (alternatively to a followup)? After all, issue #38159 will stay open independently.

dimpase commented 4 months ago

closing in favour of #38159

dimpase commented 4 months ago

@S17A05 - sorry for not realising that #38159 is perfectly good to track this further. mea culpa.