scikit-build-core icon indicating copy to clipboard operation
scikit-build-core copied to clipboard

Some tests on Windows fail with ProcessNotRegisteredError

Open lorepirri opened this issue 1 year ago • 15 comments

When building the package for conda, the following tests in tests/test_cmake_config.py fail on Windows:

  1. test_cmake_paths
  2. test_init_cache
  3. test_cmake_args

at

config.configure()

with error ProcessNotRegisteredError:

Running e.g.:

pytest -vv -ra --showlocals -k "test_cmake_args"
...
            if not cls.process_list[-1]._allow_unregistered:
>               raise exceptions.ProcessNotRegisteredError(
                    "The process '%s' was not registered."
                    % (
                        command
                        if isinstance(command, str)
                        else " ".join(str(item) for item in command),
                    )
                )
E               pytest_subprocess.exceptions.ProcessNotRegisteredError: The process '%PREFIX%\Library\bin\cmake.EXE -S%SRC_DIR%\tests\packages\simple_pure -BC:\Users\buildu.

cls        = <class 'pytest_subprocess.process_dispatcher.ProcessDispatcher'>
command    = ['C:\\Users\\builidusername\\miniconda3\\envs\\c3i\\conda-bld\\scikit-build-core_1722583656353\\_test_env\\Library\\bin\\cmake.EXE',
 '-SC:\\Users\\builidusername\\miniconda3\\envs\\c3i\\conda-bld\\scikit-build-core_1722583656353\\test_tmp\\tests\\packages\\simple_pure',
 '-BC:\\Users\\builidusername\\AppData\\Local\\Temp\\pytest-of-builidusername\\pytest-86\\test_cmake_args0\\build',
 '-DCMAKE_BUILD_TYPE:STRING=Release',
 '-DSOMETHING=one']
command_instance = None
process_instance = None
processes  = None
...

full log: scikit-build-core-0.9.9-pytest.log

Something is not working with the parameters in the registration of the commands here:

https://github.com/scikit-build/scikit-build-core/blob/715777d94d57ab0f9ae158f8690d609356a69753/tests/test_cmake_config.py#L140

https://github.com/scikit-build/scikit-build-core/blob/715777d94d57ab0f9ae158f8690d609356a69753/tests/test_cmake_config.py#L110

https://github.com/scikit-build/scikit-build-core/blob/715777d94d57ab0f9ae158f8690d609356a69753/tests/test_cmake_config.py#L56

as a test, if I replace *cmd with fp.any() to ignore the parameters, the registration works and the three tests pass.

On other platforms (linux, darwin) these three tests pass.

I could not determine if it was a \\ problem or something else.

scikit-build-core version: 0.9.9 or 0.9.8 Python: 3.8 to 3.11 Windows: 11 pytest-subprocess: 1.5.0 cmake: 3.26.4 Compiler: vs2019 win-64

lorepirri avatar Aug 02 '24 07:08 lorepirri

The versions of the dependencies would be useful in this case. It seems that fp.program didn't expand the executable path correctly.

Maybe can also check on https://github.com/conda-forge/scikit-build-core-feedstock, there the tests don't seem to be covered properly

LecrisUT avatar Aug 02 '24 08:08 LecrisUT

Thank you for your fast reply!

Apologies, here is the list of the dependencies used in the test environment:

    attrs:              23.1.0-py312haa95532_0
    blas:               1.0-mkl
    bzip2:              1.0.8-h2bbff1b_6
    ca-certificates:    2024.7.2-haa95532_0
    cattrs:             23.1.2-py312haa95532_0
    cmake:              3.26.4-h693b641_0
    colorama:           0.4.6-py312haa95532_0
    distlib:            0.3.8-py312haa95532_0
    editables:          0.3-py312haa95532_0
    expat:              2.6.2-hd77b12b_0
    filelock:           3.13.1-py312haa95532_0
    hatch-vcs:          0.3.0-py312haa95532_1
    hatchling:          1.21.1-py312haa95532_0
    iniconfig:          1.1.1-pyhd3eb1b0_0
    intel-openmp:       2023.1.0-h59b6b97_46320
    libffi:             3.4.4-hd77b12b_1
    libuv:              1.48.0-h827c3e9_0
    lz4-c:              1.9.4-h2bbff1b_1
    markdown-it-py:     2.2.0-py312haa95532_1
    mdurl:              0.1.0-py312haa95532_0
    mkl:                2023.1.0-h6b88ed4_46358
    mkl-service:        2.4.0-py312h2bbff1b_1
    mkl_fft:            1.3.8-py312h2bbff1b_0
    mkl_random:         1.2.4-py312h59b6b97_0
    ninja:              1.10.2-haa95532_5
    ninja-base:         1.10.2-h6d14046_5
    numpy:              1.26.4-py312hfd52020_0
    numpy-base:         1.26.4-py312h4dde369_0
    openssl:            3.0.14-h827c3e9_0
    packaging:          24.1-py312haa95532_0
    pathspec:           0.10.3-py312haa95532_0
    pip:                24.0-py312haa95532_0
    platformdirs:       3.10.0-py312haa95532_0
    pluggy:             1.0.0-py312haa95532_1
    pybind11:           2.12.0-py312h59b6b97_0
    pybind11-global:    2.12.0-py312h59b6b97_0
    pygments:           2.15.1-py312haa95532_1
    pyproject-metadata: 0.7.1-py312haa95532_0
    pyproject_hooks:    1.0.0-py312haa95532_0
    pytest:             7.4.4-py312haa95532_0
    pytest-subprocess:  1.5.0-py312haa95532_0
    python:             3.12.4-h14ffc60_1
    python-build:       0.10.0-py312haa95532_0
    rich:               13.7.1-py312haa95532_0
    scikit-build-core:  0.9.9-py312h0158946_0      local
    setuptools:         69.5.1-py312haa95532_0
    setuptools-scm:     8.1.0-py312haa95532_0
    sqlite:             3.45.3-h2bbff1b_0
    tbb:                2021.8.0-h59b6b97_0
    tk:                 8.6.14-h0416ee5_0
    trove-classifiers:  2023.10.18-py312haa95532_0
    tzdata:             2024a-h04d1e81_0
    vc:                 14.2-h2eaa2aa_4
    virtualenv:         20.26.1-py312haa95532_0
    vs2015_runtime:     14.29.30133-h43f2093_4
    vs2019_win-64:      19.29.30154-h96f319f_4
    vswhere:            2.8.4-haa95532_0
    wheel:              0.43.0-py312haa95532_0
    xz:                 5.4.6-h8cc25b3_1
    zlib:               1.2.13-h8cc25b3_1
    zstd:               1.5.5-hd43e919_2

lorepirri avatar Aug 02 '24 08:08 lorepirri

There is a more recent version of pytest-subprocess: 1.5.0 but It does not seem to have changes regarding the expansion of the executable path: https://github.com/aklajnert/pytest-subprocess/compare/1.5.0...1.5.2

lorepirri avatar Aug 02 '24 08:08 lorepirri

Indeed. From what I see in the CI jwlawson/actions-setup-cmake is used to setup cmake on the windows CI. Hopefully there's nothing funky there where it strips the .exe suffix and we are not covering that. We are also using vanilla windows runners though so it shouldn't be the case. 🤔

LecrisUT avatar Aug 02 '24 08:08 LecrisUT

The thing that looks weird to me is the un-expanded environment variables. %PREFIX%\Library\bin\cmake.EXE instead of C:\\Users\\builidusername\\miniconda3\\envs\\c3i\\conda-bld\\scikit-build-core_1722583656353\\_test_env\\Library\\bin\\cmake.EXE. If pytest-subprocess is just comparing strings, those are not the same string.

Edit: oh, I can't read. That's what you were talking about...

henryiii avatar Aug 02 '24 16:08 henryiii

The scikit-build-core feedstock only tests Linux, as it's hard to get it to test pure-Python feedstocks on macOS and Windows.

henryiii avatar Aug 02 '24 16:08 henryiii

Could you try https://github.com/scikit-build/scikit-build-core/pull/844 ?

henryiii avatar Aug 02 '24 17:08 henryiii

Could you try #844 ?

I did try it. It did not work.

I believe the expansion might happen but with this problem: https://github.com/conda/conda-build/issues/4264#issuecomment-1050817754

Or some other thing weird is happening.

If I have:

    print("---config.source_dir-----")
    print(str(config.source_dir))
    print(repr(config.source_dir))
    print("--------")

    for c in configure_args(config):
        print(str(c), "****", repr(c))
    print("--------")

    cmd = [os.fspath(c) for c in configure_args(config)]
    print("---cmd-----")
    print(cmd)
    print("--------")
    print("---*cmd-----")
    print(*cmd)
    print("--------")

I will see printed:

---config.source_dir-----
%SRC_DIR%\tests\packages\simple_pure
WindowsPath('C:/Users/lorepirri/miniconda3/envs/c3i/conda-bld/scikit-build-core_1722646771709/test_tmp/tests/packages/simple_pure')
--------
-S%SRC_DIR%\tests\packages\simple_pure **** '-SC:\\Users\\lorepirri\\miniconda3\\envs\\c3i\\conda-bld\\scikit-build-core_1722646771709\\test_tmp\\tests\\packages\\simple_pure'
-BC:\Users\lorepirri\AppData\Local\Temp\pytest-of-lorepirri\pytest-104\test_cmake_args0\build **** '-BC:\\Users\\lorepirri\\AppData\\Local\\Temp\\pytest-of-lorepirri\\pytest-104\\test_cmake_args0\\build'
--------
---cmd-----
['-SC:\\Users\\lorepirri\\miniconda3\\envs\\c3i\\conda-bld\\scikit-build-core_1722646771709\\test_tmp\\tests\\packages\\simple_pure', '-BC:\\Users\\lorepirri\\AppData\\Local\\Temp\\pytest-of-lorepirri\\pytest-104\\test_cmake_args0\\build']
--------
---*cmd-----
-S%SRC_DIR%\tests\packages\simple_pure -BC:\Users\lorepirri\AppData\Local\Temp\pytest-of-lorepirri\pytest-104\test_cmake_args0\build
--------

lorepirri avatar Aug 03 '24 01:08 lorepirri

Oh yeah forgot about that azure/conda "feature". They just replace in the generated log the known values of environment variables. It does not make things confusing at all 😑.

LecrisUT avatar Aug 03 '24 02:08 LecrisUT

o the two paths are different?

henryiii avatar Aug 03 '24 03:08 henryiii

Ah, we are seeing the same string twice, and it's replacing one of them. We aren't told what has been registered here.

henryiii avatar Aug 03 '24 03:08 henryiii

How hard is it to set up in CI?

henryiii avatar Aug 03 '24 03:08 henryiii

We could run a modified version of conda-build next week to disable that annoying log replacement and see if we can get the real paths.

Thank you all for helping out!

I found the problem. I apologize I got hallucinated by the nice conda feature and thought that the problem was in the expansion of the path, and I stopped there.

I am also grateful for the truncated log that conda build offers us.

There is a parameter too much: -DCMAKE_BUILD_TYPE:STRING=Release

The function configure_args in the tests that is used to prepare the arguments does not consider that there might be a CMAKE_GENERATOR set in the environment (and I have it set to Ninja, for good or bad):

https://github.com/scikit-build/scikit-build-core/blob/901b1d31d9b7b47ee93aa2ef6f887c15e82e7f7a/tests/test_cmake_config.py#L21-L30

The configure() in src/scikit_build_core/cmake.py mutates single_config just right before calling the Run(...).live(...)

https://github.com/scikit-build/scikit-build-core/blob/901b1d31d9b7b47ee93aa2ef6f887c15e82e7f7a/src/scikit_build_core/cmake.py#L210-L212

I tested this code in tests/test_cmake_config.py and it works:

def configure_args(
        config: CMaker,
        *,
        init: bool = False,
        defines: Mapping[str, str | os.PathLike[str] | bool] | None = None,
        cmake_args: Sequence[str] = (),) -> Generator[str, None, None]:
    yield f"-S{config.source_dir}"
    yield f"-B{config.build_dir}"

    _cmake_args = config._compute_cmake_args(defines or {})
    all_args = [*_cmake_args, *cmake_args]

    gen = config.get_generator(*all_args)
    if gen:
        single_config = gen == "Ninja" or "Makefiles" in gen

    if (single_config or config.single_config) and config.build_type:
        yield f"-DCMAKE_BUILD_TYPE:STRING={config.build_type}"

    if init:
        cmake_init = config.build_dir / "CMakeInit.txt"
        yield f"-C{cmake_init}"

I don't know if it's the best solution for you. One might think of extracting the part leading to mutating self.single_config into another method and using that from the tests.

If it is, please let me know and I will fix the PR with this change.

On the other hand, it might not be ideal to have the environmental variable CMAKE_GENERATOR set for testing.

lorepirri avatar Aug 03 '24 08:08 lorepirri

I think we probably should monkeypatch the test to remove the envvar. If it’s important, we could also make a new or parameterized test with the envvar set. You generally don’t want a test to depend on the environment and run differently.

henryiii avatar Aug 03 '24 14:08 henryiii

I think we probably should monkeypatch the test to remove the envvar. If it’s important, we could also make a new or parameterized test with the envvar set. You generally don’t want a test to depend on the environment and run differently.

Thank you @henryiii.

PR #847 should set things right for this particular problem. Let me know if the solution is acceptable.

lorepirri avatar Aug 04 '24 11:08 lorepirri

Is this fixed in the latest release?

henryiii avatar Aug 13 '24 01:08 henryiii

Yes, thanks for working on it and merging it!

lorepirri avatar Aug 20 '24 05:08 lorepirri