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

spkg-configure.m4 for python: tzdata, pytest, vcversioner #36674

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

dimpase
Copy link
Member

@dimpase dimpase commented Nov 7, 2023

also, remove leftovers of setuptools_scm_git_archive, left in the tree by an oversight.

This is continuation of #36332

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 7, 2023

Note that #36129 is going to remove vcversioner, so perhaps this part can be omitted

@orlitzky
Copy link
Contributor

orlitzky commented Nov 8, 2023

tzdata can be skipped in most cases because it provides timezone data that's already installed on every system; the system python will be configured to use the system timezone data. In Gentoo the tzdata package literally does nothing.

To check, we would need to run some zoneinfo command to see if it fails. I once started to write an autoconf macro for that but it's probably overkill here. Something like python -s -c 'import zoneinfo; print(zoneinfo.ZoneInfo("America/New_York"));'could work? (The -s is to simulate our runtime poisoning of PYTHONUSERBASE.)

@orlitzky
Copy link
Contributor

orlitzky commented Nov 8, 2023

"python" should probably be $PYTHON_FOR_VENV in that command, but I don't remember what the difference is any more

@orlitzky
Copy link
Contributor

Maybe we don't have to think about tzdata at all. It's used only by pytz_deprecation_shim, which is used only by tzlocal, which is used only by rpy2. Both pytz_deprecation_shim and tzlocal are already disabled with --disable-r. I think we should add tzdata to the list of R packages in configure.ac and forget about it until there's enough consensus on sage-devel to delete R.

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 13, 2023

I think we should add tzdata to the list of R packages in configure.ac

I agree

@orlitzky
Copy link
Contributor

Rebase on top of #36129 with no vcversioner and with tzdata disabled by --disable-r?

I think it's about time we switch the default to --disable-r too. The last meaningful change to the R interface was,

commit 2c069c9c1224b2f9f26b9ae282e6ac22991dfa91
Author: Karl-Dieter Crisman <kcrisman@gmail.com>
Date:   Mon Apr 19 21:26:35 2010 -0400

    Trac 7665 - updates R interface to fully enable R graphics in nb and command line

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 28, 2023

Rebase on top of #36129 with no vcversioner and with tzdata disabled by --disable-r?

Yes please

I think it's about time we switch the default to --disable-r too. The last meaningful change to the R interface was,

Hm? No, we have working rpy2, which is installed whenever there's a compatible system R.

@orlitzky
Copy link
Contributor

I think it's about time we switch the default to --disable-r too. The last meaningful change to the R interface was,

Hm? No, we have working rpy2, which is installed whenever there's a compatible system R.

And when there's not, we build one for no reason.

What I should have said though is "switch R to optional" rather than disabled. Then if people want to use R, they install R.

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 28, 2023

I think it's about time we switch the default to --disable-r too. The last meaningful change to the R interface was,

Hm? No, we have working rpy2, which is installed whenever there's a compatible system R.

And when there's not, we build one for no reason.

No, we don't. R is a dummy package.

@orlitzky
Copy link
Contributor

No, we don't. R is a dummy package.

Of course it is. I'm going to take a nap.

@orlitzky
Copy link
Contributor

In addition to adding tzdata to --disable-r, would it help to copy this REQUIRED-CHECK from rpy2?

SAGE_SPKG_CONFIGURE([rpy2], [
    SAGE_PYTHON_PACKAGE_CHECK([rpy2])
], [dnl REQUIRED-CHECK
    AC_REQUIRE([SAGE_SPKG_CONFIGURE_R])
    dnl rpy2 is only needed when there is a usable system R
    AS_VAR_IF([sage_spkg_install_r], [yes], [dnl
        AS_VAR_IF([sage_use_system_r], [installed], [dnl
            dnl Legacy SPKG installation of r
            AS_VAR_SET([SPKG_REQUIRE], [yes])
        ], [dnl No system package, no legacy SPKG installation
            AS_VAR_SET([SPKG_REQUIRE], [no])
        ])
    ])
])

It looks like tzlocal and pytz_deprecation_shim could benefit too, for the people who don't always pass --disable-r.

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 6, 2023

Should rebase and remove vcversioner now

Copy link

Documentation preview for this PR (built with commit 9959f98; changes) is ready! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants