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

distutils.ccompiler: Make has_function work with more C99 compilers #195

Merged
merged 1 commit into from
Feb 6, 2023
Merged

distutils.ccompiler: Make has_function work with more C99 compilers #195

merged 1 commit into from
Feb 6, 2023

Conversation

fweimer-rh
Copy link
Contributor

C99 removed support for implicit function declarations. This means that just calling a function, without declaring the function first, can result in a compilation error. Today, has_function works with most compilers because they issue just a warning, create an object file, and attempt a link, which then detects available of the symbol at link time, as intended. With future compilers, compilation will already fail, and no link test is performed.

The has_function interface provides the caller with a way to supply a list of header files to include. However, even with today's compilers, this only works if the function does not expect any parameters. Otherwise, the function call in the C fragment created by has_function will not supply the correct argument list and fail compilation. Therefore, this change supplies and incorrect prototype without arguments. This is what autoconf does today in a very similar situation, so it is quite likely that compilers will support this construct in this context in the future.

The includes and include_dirs arguments are deprecated because of the parameter list mismatch issue.

Fixes pypa/setuptools#3648.

C99 removed support for implicit function declarations.  This means
that just calling a function, without declaring the function first,
can result in a compilation error.  Today, has_function works with
most compilers because they issue just a warning, create an object
file, and attempt a link, which then detects available of the symbol
at link time, as intended.  With future compilers, compilation will
already fail, and no link test is performed.

The has_function interface provides the caller with a way to supply
a list of header files to include.  However, even with today's
compilers, this only works if the function does not expect any
parameters.  Otherwise, the function call in the C fragment created
by has_function will not supply the correct argument list and fail
compilation.  Therefore, this change supplies and incorrect prototype
without arguments.  This is what autoconf does today in a very
similar situation, so it is quite likely that compilers will support
this construct in this context in the future.

The includes and include_dirs arguments are deprecated because of
the parameter list mismatch issue.

Fixes pypa/setuptools#3648.
@fweimer-rh
Copy link
Contributor Author

Ping? This issue breaks setting of LIB_RT_AVAILABLE with C99 in yappi:

    if compiler.has_function('timer_create', libraries=('rt', )):
        user_macros.append(('LIB_RT_AVAILABLE', '1'))
        user_libraries.append('rt')

https://github.com/sumerc/yappi/blob/38afdacf526410f970afc58e147c7377c6c7112c/setup.py#L26

@fweimer-rh
Copy link
Contributor Author

Here is another broken package:

ceph_version_map = collections.OrderedDict(sorted({
    "hammer": "rados_pool_get_base_tier",
    "jewel": "rados_inconsistent_pg_list",
    "kraken": "rados_aio_exec",
    "luminous": "rados_read_op_omap_get_keys2",
}.items(), key=lambda t: t[0]))


def log(msg):
    print(msg, file=sys.stderr)


def setup_hook(cmd_obj, version=None):
    if version == "latest":
        version = sorted(ceph_version_map.keys())[-1]
    elif version is None:
        comp = ccompiler.new_compiler(force=True, verbose=True)
        for potential_version, method in ceph_version_map.items():
            msg = "* checking for librados >= %s (with function %s)" % (
                potential_version, method)
            log(msg)
            found = comp.has_function(method, libraries=['rados'])
…

https://github.com/sileht/pycradox/blob/master/setup.py#L16

@fweimer-rh
Copy link
Contributor Author

@jaraco jaraco merged commit 8bf0435 into pypa:main Feb 6, 2023
jaraco added a commit that referenced this pull request Feb 6, 2023
@jaraco
Copy link
Member

jaraco commented Feb 6, 2023

I missed that this PR hadn't run CI tests successfully. The tests are failing on Windows. In 2f16327, I've marked the test as failing on Windows. Any chance you can adopt the test to also support Windows?

@fweimer-rh
Copy link
Contributor Author

I missed that this PR hadn't run CI tests successfully. The tests are failing on Windows. In 2f16327, I've marked the test as failing on Windows. Any chance you can adopt the test to also support Windows?

Huh. Do you have a link to the build and tests logs?

@jaraco
Copy link
Member

jaraco commented Feb 6, 2023

I was basing my assessment on this run.

@fweimer-rh
Copy link
Contributor Author

Looks like the test is producing a.out.exe:

INFO     root:spawn.py:38 "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.34.31933\bin\HostX86\x64\link.exe" /nologo /INCREMENTAL:NO /LTCG /MANIFEST:EMBED,ID=1 "/LIBPATH:C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.34.31933\ATLMFC\lib\x64" "/LIBPATH:C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.34.31933\lib\x64" "/LIBPATH:C:\Program Files (x86)\Windows Kits\NETFXSDK\4.8\lib\um\x64" "/LIBPATH:C:\Program Files (x86)\Windows Kits\10\lib\10.0.22621.0\ucrt\x64" "/LIBPATH:C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\x64" C:\Users\RUNNER~1\AppData\Local\Temp\abortwkowtay8.obj /OUT:a.out.exe

But is trying to delete a.out:

            else:
>               os.remove(os.path.join(self.output_dir or '', "a.out"))
E               FileNotFoundError: [WinError 2] The system cannot find the file specified: 'a.out'

This looks like a pre-existing bug? This must be the first test in the test suite that expects a successful has_function result on Windows. This may be the reason why the previous abort test was in distutils/tests/test_unixccompiler.py.

It probably needs to use CCompiler.executable_filename to construct the file name to be deleted.

@fweimer-rh
Copy link
Contributor Author

Let's see how #203 fares (I didn't even test it locally, sorry).

inmantaci pushed a commit to inmanta/inmanta-core that referenced this pull request Feb 7, 2023
Bumps [setuptools](https://github.com/pypa/setuptools) from 67.1.0 to 67.2.0.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a href="https://github.com/pypa/setuptools/blob/main/CHANGES.rst">setuptools's changelog</a>.</em></p>
<blockquote>
<h2>v67.2.0</h2>
<p>Changes
^^^^^^^</p>
<ul>
<li><a href="https://github-redirect.dependabot.com/pypa/setuptools/issues/3809">#3809</a>: Merge with distutils@8c3c3d29, including fix for <code>sysconfig.get_python_inc()</code><code>pypa/distutils#178</code><code>has_function</code><code>pypa/distutils#195</code></li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/pypa/setuptools/commit/f75c6289666ae845b80f1666ea68ec0f28afcc73"><code>f75c628</code></a> Bump version: 67.1.0 → 67.2.0</li>
<li><a href="https://github.com/pypa/setuptools/commit/bb17d4f9b23b3b8d670271f36d8894239295efe3"><code>bb17d4f</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/setuptools/issues/3809">#3809</a> from pypa/distutils-8c3c3d29</li>
<li><a href="https://github.com/pypa/setuptools/commit/47c7cfd67862e65eb19dca8b60094db0fe203049"><code>47c7cfd</code></a> Add changelog (draft)</li>
<li><a href="https://github.com/pypa/setuptools/commit/43c6ee4b2b2ce4637bb1a36400fbaa548150ec8b"><code>43c6ee4</code></a> Merge <a href="https://github.com/pypa/distutils">https://github.com/pypa/distutils</a> into distutils-8c3c3d29</li>
<li><a href="https://github.com/pypa/setuptools/commit/8c3c3d29bda85afb7f86c23a8ba66f7519f79e88"><code>8c3c3d2</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/setuptools/issues/199">#199</a> from mrbean-bremen/issue3591</li>
<li><a href="https://github.com/pypa/setuptools/commit/1efd3d54c39aab7f960f7934298fbab4cff205be"><code>1efd3d5</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/setuptools/issues/201">#201</a> from mattip/pypy2</li>
<li><a href="https://github.com/pypa/setuptools/commit/854862f228aa727b8ad88e1c432027767aff9db1"><code>854862f</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/setuptools/issues/203">#203</a> from fweimer-rh/ccompiler-windows</li>
<li><a href="https://github.com/pypa/setuptools/commit/8fe7b5f2eee8a1eea501be3635e6e7af9553db6d"><code>8fe7b5f</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/setuptools/issues/202">#202</a> from GalaxySnail/fix-mingw-w64-2</li>
<li><a href="https://github.com/pypa/setuptools/commit/7e751d33d42de2ce9bdb08dcd0a1bd5d5b85eed9"><code>7e751d3</code></a> distutils.ccompiler: Remove correct executable file on Windows</li>
<li><a href="https://github.com/pypa/setuptools/commit/2f16327060394b21fadb6342ea83eec324512aaa"><code>2f16327</code></a> Mark test as xfail on Windows. Ref <a href="https://github-redirect.dependabot.com/pypa/distutils/issues/195">pypa/distutils#195</a>.</li>
<li>Additional commits viewable in <a href="https://github.com/pypa/setuptools/compare/v67.1.0...v67.2.0">compare view</a></li>
</ul>
</details>
<br />

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=setuptools&package-manager=pip&previous-version=67.1.0&new-version=67.2.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

</details>
clrpackages pushed a commit to clearlinux-pkgs/pypi-setuptools that referenced this pull request Feb 7, 2023
…version 67.2.0

Dimitri Papadopoulos (8):
      Add `tests/test*.py` to source distributions
      Apply refurb suggestions
      Apply refurb suggestions
      Apply refurb suggestions
      Apply refurb suggestions
      Apply refurb suggestions
      Update outdated GitHub Actions
      Fix typos found by codespell

Florian Weimer (2):
      distutils.ccompiler: Make has_function work with more C99 compilers
      distutils.ccompiler: Remove correct executable file on Windows

GalaxySnail (2):
      Fix MinGW-w64 segmentation fault
      Fix MinGW-w64 segmentation fault

Hugo van Kemenade (1):
      Link directly to PEPs

Jason R. Coombs (7):
      Add xfail test capturing missed expectation. Ref pypa/distutils#178.
      In _get_python_inc_posix, only return an extant path from the sysconfig. Fixes pypa/distutils#178.
      ⚫ Fade to black.
      Revert "Merge pull request #197 from GalaxySnail/fix-mingw-w64"
      Mark test as xfail on Windows. Ref pypa/distutils#195.
      Add changelog (draft)
      Bump version: 67.1.0 → 67.2.0

mattip (1):
      add a pypy CI run

mrbean-bremen (1):
      Fixed accumulating include dirs after compile
@mgorny
Copy link
Contributor

mgorny commented Feb 13, 2023

@fweimer-rh, do you happen to have a version of this patch that applies to CPython's version of distutils? If not, don't worry. I was mostly wondering if RedHat's planning to patch the distutils from stdlib as well.

@fweimer-rh
Copy link
Contributor Author

@mgorny I don't have such a patch yet. I'm still slightly confused whether we need it, either for our distribution package builds or pip usage.

@hroncok
Copy link
Contributor

hroncok commented Feb 13, 2023

We are considering to do that. Do I assume correctly you do as well? Maybe we can convince upstream to take the change into Pytho. 3.11.3?

@mgorny
Copy link
Contributor

mgorny commented Feb 13, 2023

@mgorny I don't have such a patch yet. I'm still slightly confused whether we need it, either for our distribution package builds or pip usage.

I'm not sure either. FWICS there are still a few packages that require forcing "stdlib" distutils but from a quick grep they don't seem to be using has_function().

We are considering to do that. Do I assume correctly you do as well? Maybe we can convince upstream to take the change into Pytho. 3.11.3?

Right now we're at "if it's easy, no harm in backporting it but we're not going to go out of our way to do that" ;-). Convincing upstream would be nice but I wouldn't hold my hopes high.

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

Successfully merging this pull request may close these issues.

Unclear how CCompiler.has_function works for functions with parameters
4 participants