bskinn / sphobjinv

Toolkit for manipulation and inspection of Sphinx objects.inv files
https://sphobjinv.readthedocs.io
MIT License
78 stars 7 forks source link

Consider uploading source dist (aka. "sdist") to PyPI #264

Closed anjos closed 1 year ago

anjos commented 1 year ago

Is your feature request related to a problem? Please describe.

I'm trying to create a conda package for this utility. The recommended way to get this done is via grayskull. For that tool to correctly operate, it requires the sdist package to be uploaded to PyPI.

Describe the solution you'd like

Include the "sdist" source package when you do PyPI uploads of new releases.

Describe alternatives you've considered

Currently, I got away with conda-packaging using GitHub's archive for extracting the tarball of the latest release. It works, but a cleaner cut would be to have the sdist regularly uploaded to PyPI.

Additional context

anjos commented 1 year ago

For that to be properly tested on conda-forge, both tests/ and conftest.py must be included with the source distribution.

bskinn commented 1 year ago

Very cool! Thanks for the work to get this up on conda -- I'd considered doing it myself, so I really appreciate you working on it.

Should be easy enough to do... I'll make sure that both conftest.py and tests/ are in the dist, as you indicate.

Logistically, what's the best way to proceed to make sure that I've packaged everything you need into the sdists? Can I provide you with tarballs via a side channel (attaching them to this issue?) for you to test, or do they have to be mediated through PyPI?

Also, which versions would you like sdists for? v2.3 as the latest, obviously -- how far back would be good?

It looks like I stopped uploading sdists after v2.0.1 (which was the point where I realized that I'd been uploading broken sdists... hm, I should look into whether I can yank individual files...), so it would be pretty clean for me to go all the way back to v2.1.

anjos commented 1 year ago

Before I test the PyPI sdists, I'd like to get a functional build, including testing. However, I'm stuck getting this package to test properly (in a conda-build environment): I have a few pytest markers missing (pytest.mark.api and pytest.mark.cli). Where are these coming from?

+ pytest -rsxX -Werror
============================= test session starts ==============================
platform darwin -- Python 3.11.0, pytest-7.2.0, pluggy-1.0.0
rootdir: $SRC_DIR
plugins: timeout-2.1.0, ordering-0.6
collected 291 items / 8 errors

==================================== ERRORS ====================================
___________________ ERROR collecting tests/test_api_fail.py ____________________
tests/test_api_fail.py:41: in <module>
    pytestmark = [pytest.mark.api, pytest.mark.local]
../_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_/lib/python3.11/site-packages/_pytest/mark/structures.py:544: in __getattr__
    warnings.warn(
E   pytest.PytestUnknownMarkWarning: Unknown pytest.mark.api - is this a typo?  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html

Any ideas?

bskinn commented 1 year ago

Mmm, those live in tox.ini -- I'll have to make sure that's in the manifest.

Ah: the doc/source directory, too -- the test suite requires a fresh build of the Sphinx docs (cd doc; make html) in order to function correctly.

Probably requirements-(dev|flake8).txt, too.

anjos commented 1 year ago

I'm finishing the bskinn/stdio-mgr packaging, which is required to test this package (I took the liberty to add you as a maintainer, since you showed interest). I'll upload that first in a bit.

anjos commented 1 year ago

Here, for your reference: https://github.com/conda-forge/staged-recipes/pull/21238

bskinn commented 1 year ago

Cool! Hopefully that v1.0.1 sdist is functional, my packaging config was incorrect for a while.

The Azure pipelines seem content enough... though, oof -- not surprising that the logging got fouled up, what with how the stdio-mgr test suite mucks about with stdio.

bskinn commented 1 year ago

One thing to watch out for, potentially -- sphobjinv >=2.2 does have a vendored dependency, an old copy of fuzzywuzzy. I do have the relevant LICENSE.txt in the source tree... I'll make sure that makes it into the sdist.

bskinn commented 1 year ago

Manifest checks:

EXCLUDE:

anjos commented 1 year ago

This package also requires pytest-check, which I just created another recipe for: https://github.com/conda-forge/staged-recipes/pull/21239 (not really, pytest_check is available here)

anjos commented 1 year ago

Thanks for all the tips. I think I have an almost OK package. I still face this:

_ TestInventory.test_api_inventory_bytes_fname_instantiation[no_op--plaintext] _

self = <tests.test_api_good.TestInventory object at 0x102813810>
source_type = <SourceTypes.BytesPlaintext: 'bytes_plain'>, inv_arg = 'plaintext'
path_fxn = <function no_op at 0x1024e68e0>
res_path = PosixPath('tests/resource')
misc_info = <conftest.misc_info.<locals>.Info object at 0x102786890>
attrs_inventory_test = <function attrs_inventory_test.<locals>.func at 0x1029980e0>
check = <pytest_check.context_manager.CheckContextManager object at 0x101a64850>

    @pytest.mark.parametrize(
        ["source_type", "inv_arg"],
        [
            (soi.SourceTypes.BytesPlaintext, "plaintext"),
            (soi.SourceTypes.BytesZlib, "zlib"),
            (soi.SourceTypes.FnamePlaintext, "fname_plain"),
            (soi.SourceTypes.FnameZlib, "fname_zlib"),
        ],
        ids=(lambda v: v if isinstance(v, str) else ""),
    )
    @pytest.mark.parametrize("path_fxn", PATH_FXNS, ids=PATH_FXN_IDS)
    def test_api_inventory_bytes_fname_instantiation(
        self,
        source_type,
        inv_arg,
        path_fxn,
        res_path,
        misc_info,
        attrs_inventory_test,
        check,
    ):
        """Check bytes and filename modes for Inventory instantiation."""
        fname = misc_info.FNames.RES

        if source_type in (
            soi.SourceTypes.BytesPlaintext,
            soi.SourceTypes.FnamePlaintext,
        ):
            fname += misc_info.Extensions.DEC
        else:
            fname += misc_info.Extensions.CMP

        source = path_fxn(res_path / fname)

        if source_type in (soi.SourceTypes.BytesPlaintext, soi.SourceTypes.BytesZlib):
            # Passing in the actual inventory contents, and not just the location
            source = soi.readbytes(source)

        # General import, without a specified kwarg
>       with check.check(msg="general"):
E       AttributeError: 'CheckContextManager' object has no attribute 'check'

Any ideas on how to fix it?

anjos commented 1 year ago

Hummm... That was due to a newer version of pytest-check being used (1.1.2). Running with 1.0.5 makes most of the tests pass. Only missing a couple to go:

_____________________________ [doctest] README.rst _____________________________
149 JSON output is supported (``sphobjinv convert json ...``), and
150 inventories can be re-compressed to the
151 partially-zlib-compressed form that ``intersphinx`` reads
152 (``sphobjinv convert zlib ...``).
153
154 Alternatively, ``sphobjinv`` exposes an API to enable automation of
155 inventory creation/modification::
156
157     >>> import sphobjinv as soi
158     >>> inv = soi.Inventory('doc/build/html/objects.inv')
UNEXPECTED EXCEPTION: TypeError('Invalid Inventory source type')
Traceback (most recent call last):
  File "$PREFIX/lib/python3.11/doctest.py", line 1350, in __run
    exec(compile(example.source, filename, "single",
  File "<doctest README.rst[1]>", line 1, in <module>
  File "<attrs generated init sphobjinv.inventory.Inventory>", line 16, in __init__
    self.__attrs_post_init__()
  File "$PREFIX/lib/python3.11/site-packages/sphobjinv/inventory.py", line 329, in __attrs_post_init__
    self._general_import()
  File "$PREFIX/lib/python3.11/site-packages/sphobjinv/inventory.py", line 565, in _general_import
    raise TypeError("Invalid Inventory source type")
TypeError: Invalid Inventory source type
$SRC_DIR/README.rst:158: UnexpectedException
____________________________ test_readme_shell_cmds ____________________________
FAILURE:

  Expected:

      Project: sphobjinv
      Version: 2.3

      219 objects in inventory.

      11 results found at/above current threshold of 58.

      Cannot infer intersphinx_mapping from a local objects.inv.

        Name                                                Score
      ---------------------------------------------------  -------
      :py:property:`sphobjinv.data.SuperDataObj.as_rst`      60
      :py:class:`sphobjinv.cli.parser.PrsConst`              59
      :py:class:`sphobjinv.data.DataFields`                  59
      :py:class:`sphobjinv.data.DataObjBytes`                59
      :py:class:`sphobjinv.data.DataObjStr`                  59
      :py:class:`sphobjinv.data.SuperDataObj`                59
      :py:class:`sphobjinv.enum.HeaderFields`                59
      :py:class:`sphobjinv.enum.SourceTypes`                 59
      :py:function:`sphobjinv.fileops.writebytes`            59
      :py:function:`sphobjinv.fileops.writejson`             59
      :py:class:`sphobjinv.inventory.Inventory`              59

  Got:

  Error while parsing input file path:
  FileNotFoundError: Indicated path is not a valid file

assert False
 +  where False = <bound method OutputChecker.check_output of <doctest.OutputChecker object at 0x1067d84d0>>('\n\n    Project: sphobjinv\n    Version: 2.3\n\n    219 objects in inventory.\n\n    11 results found at/above curren...function:`sphobjinv.fileops.writejson`             59\n    :py:class:`sphobjinv.inventory.Inventory`              59\n', ' \n\nError while parsing input file path:\nFileNotFoundError: Indicated path is not a valid file\n', 12)
 +    where <bound method OutputChecker.check_output of <doctest.OutputChecker object at 0x1067d84d0>> = <doctest.OutputChecker object at 0x1067d84d0>.check_output
tests/test_readme.py:92 in test_readme_shell_cmds() -> with check.check():
FAILURE:

  Expected:

      Conversion completed.
      '...objects.inv' converted to '...objects.txt' (plain).

  Got:

  Error while parsing input file path:
  FileNotFoundError: Indicated path is not a valid file

assert False
 +  where False = <bound method OutputChecker.check_output of <doctest.OutputChecker object at 0x1067d84d0>>("\n    Conversion completed.\n    '...objects.inv' converted to '...objects.txt' (plain).\n", ' \n\nError while parsing input file path:\nFileNotFoundError: Indicated path is not a valid file\n', 12)
 +    where <bound method OutputChecker.check_output of <doctest.OutputChecker object at 0x1067d84d0>> = <doctest.OutputChecker object at 0x1067d84d0>.check_output
tests/test_readme.py:92 in test_readme_shell_cmds() -> with check.check():
anjos commented 1 year ago

For the time being, I'm ignoring the tests with pytest -k 'not readme'.

anjos commented 1 year ago

I have a working package. The vendored dependence fuzzywuzzy (version 0.18, 0.17 and 0.16) is available in conda-forge, and as per-rules we should not include it. Any ideas on how to sort this?

bskinn commented 1 year ago

For the time being, I'm ignoring the tests with pytest -k 'not readme'.

Please do. v2.3.1 will exclude the README tests by default, per #260 and #261. They'll now only be run in a dedicated job in Azure Pipelines. Better for automated test situations like this, and also less annoyance in my dev workflow.

Hummm... That was due to a newer version of pytest-check being used (1.1.2).

Yep, I saw @okken's toot about the change in pytest-check behavior. Added #265 to track.

anjos commented 1 year ago

For the time being, I'm ignoring the tests with pytest -k 'not readme'.

Please do. v2.3.1 will exclude the README tests by default, per #260 and #261. They'll now only be run in a dedicated job in Azure Pipelines. Better for automated test situations like this, and also less annoyance in my dev workflow.

Hummm... That was due to a newer version of pytest-check being used (1.1.2).

Yep, I saw @okken's toot about the change in pytest-check behavior. Added #265 to track.

I'm pinning the version of pytest-check to avoid issues for now. We can review that dependence when you have tackled #265.

bskinn commented 1 year ago

The vendored dependence fuzzywuzzy (version 0.18, 0.17 and 0.16) is available in conda-forge, and as per-rules we should not include it. Any ideas on how to sort this?

The problem with fuzzywuzzy is a license conflict. Everything after v0.3.0 is licensed GPL, which is incompatible with sphobjinv's MIT. I don't see any immediate solution that doesn't involve vendoring: unless I did something wrong, fuzzywuzzy v0.3.0 is incompatible with Python 3 -- I had to modify it to make it work.

anjos commented 1 year ago

Right. I'll argue for it in case it is brought up during the package review. I'll ping you then.

bskinn commented 1 year ago

If I stick to my current roadmap, once I get v2.4 out, fuzzywuzzy will be moot: I'm planning to completely re-implement the fuzzywuzzy algorithm in a more multiprocessing-friendly way (part of #178), which is also an early step toward pluggable suggest scorers (#207).

okken commented 1 year ago

Thanks for tagging me on this. I intended the change to be completely backwards compatible. I'm sorry that this has caused you work. Probably is for other people also. If you have time, could you explain what the issue is? or is it easiest for me to just clone the repo and run tests?

bskinn commented 1 year ago

@okken I haven't looked at it myself yet; the error that showed is above, here: https://github.com/bskinn/sphobjinv/issues/264#issuecomment-1323935399

bskinn commented 1 year ago

It appears I set up pytest-check as a fixture... for some reason. Don't remember why.

bskinn commented 1 year ago

Oh. That might just be builtin functionality: check = <pytest_check.context_manager.CheckContextManager object at 0x101a64850>

okken commented 1 year ago

There's a bunch of things like this

def test_(check):
   with check.check:
       ...

You can now replace with check.check: with with check:.

However, I'm sorry I broke stuff. I'll file an issue and try to get it to be more backward compatible. However, ... I did try your repo tests with searching for all "check.check" and replacing with "check" and it fixed it.

bskinn commented 1 year ago

Yup, that's what I was expecting the fix to be, from the sense I got from your toot. I'll definitely migrate everything to the new syntax in the next release. Thanks for the diagnostic! And, no worries -- it happens.

okken commented 1 year ago

Fixed in https://github.com/okken/pytest-check/pull/97 This published just now as version 1.1.3

bskinn commented 1 year ago

:+1: Thanks!

bskinn commented 1 year ago

@anjos: The updated MANIFEST.in seems to be providing promising results.

What do you need from me to test a shiny, newly-filled-out sdist? Can I just attach the tarball here, or do you need me to cut a (pre)release on PyPI?

anjos commented 1 year ago

@bskinn thanks for all the reactivity on this issue. I suppose I can work with a .tar.gz of your prototype.

bskinn commented 1 year ago

Happy to do it -- I have the bandwidth at the moment, so might as well dive in.

I figure if an sdist passed this way doesn't work, then good odds a PyPI sdist won't either. Better to debug this way before going through my full release process.

Here you go: sphobjinv-2.3.1.dev1.tar.gz

anjos commented 1 year ago

Brian, with the package you provided, and pytest-check==1.1.3, I get 12 errors that look like this:

_____________________ TestCore.test_api_decompress[no_op] ______________________

self = <tests.test_api_good.TestCore object at 0x1030630d0>
path_fxn = <function no_op at 0x102d16840>
scratch_path = PosixPath('/private/var/folders/y4/n6m6rkls6zs72x8jd2w2tlx40000gn/T/pytest-of-andre/pytest-13/test_api_decompress_no_op_0')
misc_info = <conftest.misc_info.<locals>.Info object at 0x102e42250>
decomp_cmp_test = <function decomp_cmp_test.<locals>.func at 0x102fb77e0>

    @pytest.mark.parametrize("path_fxn", PATH_FXNS, ids=PATH_FXN_IDS)
    def test_api_decompress(self, path_fxn, scratch_path, misc_info, decomp_cmp_test):
        """Check that a decompress attempt via API throws no errors."""
        src_path = scratch_path / (misc_info.FNames.INIT + misc_info.Extensions.CMP)
        dest_path = scratch_path / (misc_info.FNames.MOD + misc_info.Extensions.DEC)

        b_cmp = soi.readbytes(path_fxn(src_path))
        b_dec = soi.decompress(b_cmp)
        soi.writebytes(path_fxn(dest_path), b_dec)

        assert dest_path.is_file()

>       decomp_cmp_test(dest_path)

tests/test_api_good.py:107:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

path = PosixPath('/private/var/folders/y4/n6m6rkls6zs72x8jd2w2tlx40000gn/T/pytest-of-andre/pytest-13/test_api_decompress_no_op_0/objects_mod.txt')

    def func(path):
        """Perform the round-trip compress/decompress comparison test."""
        # The str() calls here are for Python 3.5 compat
>       assert cmp(str(misc_info.res_decomp_path), str(path), shallow=False)
E       AssertionError: assert False
E        +  where False = cmp('tests/resource/objects_attrs.txt', '/private/var/folders/y4/n6m6rkls6zs72x8jd2w2tlx40000gn/T/pytest-of-andre/pytest-13/test_api_decompress_no_op_0/objects_mod.txt', shallow=False)
E        +    where 'tests/resource/objects_attrs.txt' = str(PosixPath('tests/resource/objects_attrs.txt'))
E        +      where PosixPath('tests/resource/objects_attrs.txt') = <conftest.misc_info.<locals>.Info object at 0x102e42250>.res_decomp_path
E        +    and   '/private/var/folders/y4/n6m6rkls6zs72x8jd2w2tlx40000gn/T/pytest-of-andre/pytest-13/test_api_decompress_no_op_0/objects_mod.txt' = str(PosixPath('/private/var/folders/y4/n6m6rkls6zs72x8jd2w2tlx40000gn/T/pytest-of-andre/pytest-13/test_api_decompress_no_op_0/objects_mod.txt'))

conftest.py:269: AssertionError

Do I miss a file?

The tests that fail are rather difficult to find in the way pytest is currently configured (I guess this comes from your tox.ini file). I'm not sure why the the failed tests are not printed by the end of the run.

Otherwise, may I suggest you move the file conftest.py into the tests/ directory? That would help reducing the clutter on the root directory, and copying specific source files on the conda recipe. So far, I have the following:

bskinn commented 1 year ago

Huh. Do all of the failing tests use the decomp_cmp_test fixture? I don't see this failure either locally or in the new sdist testing job on Azure Pipelines (Ubuntu).

Can you attach the temp file from /private/var/folders/y4/... for this test?

bskinn commented 1 year ago

First impression, this seems like an OS or environment specific quirk

anjos commented 1 year ago

Here it is: pytest-output.tar.gz

anjos commented 1 year ago

W.r.t. conda-forge integration, we are just down to this package. I'll make a request tomorrow.

The use of the source from PyPI can be integrated later.

bskinn commented 1 year ago

Took a close look at the before and after files here. I think I know what's happening.

The tests/resource/ contains a decompressed inventory for use in the tests. I built this tarball on Windows, which means that text files were checked out with Windows \r\n in that local directory tree. And, thus, since the tarball was built directly from these files, and not passed through git's automatic EOL conversion, this Windows-EOL text file was packaged with its Windows EOLs intact. Those extra \rs are fouling the comparison.

$ diff -y objects.txt objects_mod.txt | less

# Sphinx inventory version 2^M                                                        | # Sphinx inventory version 2
# Project: attrs^M                                                            | # Project: attrs
# Version: 22.1^M                                                             | # Version: 22.1
# The remainder of this file is compressed using zlib.^M                                                              | # The remainder of this file is compressed using zlib.
attr py:module 0 index.html#module-$ -^M                                                              | attr py:module 0 index.html#module-$ -
attr.VersionInfo py:class 1 api.html#$ -^M                                                            | attr.VersionInfo py:class 1 api.html#$ -
...

So, the solution is to dos2unix that file, and then declare that .txt inventory file as binary to git using .gitattributes, so that its newlines are never converted from UNIX. New tarball inbound shortly.

bskinn commented 1 year ago

Right, so it wasn't quite that simple--I had to add some handling to cope with that file now having Unix EOLs even on Windows--but that was definitely the core problem. 🤞 that the CI is happy now.

anjos commented 1 year ago

Alright - let me know when I can have a second tar ball with the fix for testing.

anjos commented 1 year ago

FYI: The conda-forge recipe is under review at conda-forge/staged-recipes#21285

bskinn commented 1 year ago

Take-two tarball:

sphobjinv-2.3.1.dev2.tar.gz

anjos commented 1 year ago

Bingo! That passes for me, using the latest pytest-check version (1.2.0). From my side, this looks ready.

Here is the test summary:

=========================== short test summary info ============================
SKIPPED [3] tests/test_api_good.py:499: '--testall' not specified
SKIPPED [3] tests/test_api_good.py:533: '--testall' not specified
SKIPPED [6] tests/test_api_good_nonlocal.py:48: '--nonloc' not specified
SKIPPED [3] tests/test_cli.py:124: Ignore no-change conversions
SKIPPED [3] tests/test_cli.py:226: '--testall' not specified
SKIPPED [12] tests/test_cli_nonlocal.py:57: '--nonloc' not specified
SKIPPED [1] tests/test_flake8_ext.py:50: '--flake8_ext' not specified
XFAIL tests/test_api_fail.py::TestImmutable::test_apifail_changing_immutable_dataobj[True-no_op] - Made mutable to simplify Inventory revision by users
XFAIL tests/test_api_fail.py::TestImmutable::test_apifail_changing_immutable_dataobj[True-str] - Made mutable to simplify Inventory revision by users
XFAIL tests/test_api_fail.py::TestImmutable::test_apifail_changing_immutable_dataobj[False-no_op] - Made mutable to simplify Inventory revision by users
XFAIL tests/test_api_fail.py::TestImmutable::test_apifail_changing_immutable_dataobj[False-str] - Made mutable to simplify Inventory revision by users
XFAIL tests/test_valid_objects.py::test_name_lead_chars[10_\n] - reason: Known invalid name lead char
XFAIL tests/test_valid_objects.py::test_name_lead_chars[35_#] - reason: Known invalid name lead char
=========== 436 passed, 31 skipped, 1 deselected, 6 xfailed in 1.17s ===========

Thanks for all the work on this!

bskinn commented 1 year ago

Cool! I'll get this merged, and get 2.3.1 out onto PyPI as soon as I can.

Thanks to you too for helping debug everything, and thanks again for taking on getting the project onto conda-forge!

anjos commented 1 year ago

Will this also sort out issues on conda-forge/staged-recipes#21285 with Windows?

bskinn commented 1 year ago

That suggested revision of mine on the PR will still need to be applied, but after that, yes, I believe all should be resolved there.

bskinn commented 1 year ago

(Resolved once 2.3.1 is up. The continued failure on 2.3 is expected.)

anjos commented 1 year ago

FYI, sphobjinv is now available on conda-forge.

bskinn commented 1 year ago

Very cool - thanks again!