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

doctests: Filter out linker warning messages #31204

Closed
mkoeppe opened this issue Jan 7, 2021 · 34 comments
Closed

doctests: Filter out linker warning messages #31204

mkoeppe opened this issue Jan 7, 2021 · 34 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Jan 7, 2021

(from #30589)

On OS X Big Sur + various homebrew packages, if I use

% ./configure --with-system-python3=no --enable-download-from-upstream-url 

then Sage builds (if I also incorporate various other changes from #30651), but I get doctest failures of the form

File "src/sage/misc/sagedoc.py", line 646, in sage.misc.sagedoc.format
Failed example:
    cython('\n'.join(cython_code))
Expected nothing
Got:
    ld: warning: dylib (/usr/local/lib/libmpfr.dylib) was built for newer macOS version (11.0) than being linked (10.9)
    ld: warning: dylib (/usr/local/lib/libgmp.dylib) was built for newer macOS version (11.0) than being linked (10.9)
    ld: warning: dylib (/usr/local/lib/libgmpxx.dylib) was built for newer macOS version (11.0) than being linked (10.9)
    ld: warning: dylib (/usr/local/lib/libgsl.dylib) was built for newer macOS version (11.0) than being linked (10.9)
    ld: warning: dylib (/usr/local/lib/libntl.dylib) was built for newer macOS version (11.0) than being linked (10.9)

We should filter out these warnings in doctesting.

CC: @jhpalmieri @dimpase @kiwifb

Component: doctest framework

Author: John Palmieri

Branch/Commit: 55cf2ee

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/31204

@mkoeppe mkoeppe added this to the sage-9.3 milestone Jan 7, 2021
@jhpalmieri
Copy link
Member

comment:1

I've only seen this on OS X. The following has worked for me, although it hasn't been thoroughly tested:

diff --git a/src/sage/doctest/parsing.py b/src/sage/doctest/parsing.py
index 4e1a6b32e4..77f726a27b 100644
--- a/src/sage/doctest/parsing.py
+++ b/src/sage/doctest/parsing.py
@@ -40,6 +40,8 @@ optional_regex = re.compile(r'(arb216|arb218|py2|py3|long time|not implemented|n
 # which has not been patched, we need to ignore that message.
 # See :trac:`29317`.
 glpk_simplex_warning_regex = re.compile(r'(Long-step dual simplex will be used)')
+# :trac:`31204` -- suppress warning about ld and OS version for dylib files.
+ld_warning_regex = re.compile(r'ld: warning: dylib.*was built for newer macOS version .* than being linked.*')
 find_sage_prompt = re.compile(r"^(\s*)sage: ", re.M)
 find_sage_continuation = re.compile(r"^(\s*)\.\.\.\.:", re.M)
 find_python_continuation = re.compile(r"^(\s*)\.\.\.([^\.])", re.M)
@@ -1087,6 +1089,7 @@ class SageOutputChecker(doctest.OutputChecker):
         """
         got = self.human_readable_escape_sequences(got)
         got = glpk_simplex_warning_regex.sub('', got)
+        got = ld_warning_regex.sub('', got)
         if isinstance(want, MarkedOutput):
             if want.random:
                 return True

I can produce a branch easily enough.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 8, 2021

comment:2

Looks fine to me. I briefly looked to find instances of this on Linux in GH Actions logs, but did not find anything, it's likely that I have confused this with something else.

@jhpalmieri
Copy link
Member

comment:3

This doesn't quite catch all of the warnings:

**********************************************************************
File "src/sage/tests/cmdline.py", line 301, in sage.tests.cmdline.test_executable
Failed example:
    err
Expected:
    'Compiling ...spyx...'
Got:
    'Compiling /Users/palmieri/.sage/temp/John-iMac-2017.local/34214/dir__zddcuog/sage_test_file.spyx...\nld: warning: dylib (/usr/local/lib/libgmp.dylib) was built for newer macOS version (11.0) than being linked (10.9)\nld: warning: dylib (/usr/local/lib/libmpfr.dylib) was built for newer macOS version (11.0) than being linked (10.9)\nld: warning: dylib (/usr/local/lib/libgmpxx.dylib) was built for newer macOS version (11.0) than being linked (10.9)\nld: warning: dylib (/usr/local/lib/libgsl.dylib) was built for newer macOS version (11.0) than being linked (10.9)\nld: warning: dylib (/usr/local/lib/libntl.dylib) was built for newer macOS version (11.0) than being linked (10.9)\n'
**********************************************************************
File "src/sage/tests/cmdline.py", line 308, in sage.tests.cmdline.test_executable
Failed example:
    err
Expected:
    'Compiling ...spyx...'
Got:
    'Compiling sage_test_file.spyx...\nld: warning: dylib (/usr/local/lib/libgmp.dylib) was built for newer macOS version (11.0) than being linked (10.9)\nld: warning: dylib (/usr/local/lib/libmpfr.dylib) was built for newer macOS version (11.0) than being linked (10.9)\nld: warning: dylib (/usr/local/lib/libgmpxx.dylib) was built for newer macOS version (11.0) than being linked (10.9)\nld: warning: dylib (/usr/local/lib/libgsl.dylib) was built for newer macOS version (11.0) than being linked (10.9)\nld: warning: dylib (/usr/local/lib/libntl.dylib) was built for newer macOS version (11.0) than being linked (10.9)\n'
**********************************************************************
1 item had failures:
   2 of 231 in sage.tests.cmdline.test_executable

I'm not sure why the "..." doesn't capture all of this. Maybe it matches the verbatim "..." in the doctest output, or maybe it doesn't capture multiline strings.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jan 8, 2021

comment:4

Could you push a branch please?

@jhpalmieri
Copy link
Member

Branch: u/jhpalmieri/31204-ld-warnings

@jhpalmieri
Copy link
Member

comment:6

Replying to @mkoeppe:

Could you push a branch please?

Sure.


New commits:

bb6edf6trac 31204: suppress ld warning messages about wrong OS X version

@jhpalmieri
Copy link
Member

Commit: bb6edf6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 31, 2021

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

801c1a7trac 31204: suppress ld warning messages about wrong OS X version

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 31, 2021

Changed commit from bb6edf6 to 801c1a7

@jhpalmieri
Copy link
Member

comment:8

I modified the filter a bit, starting it with "dylib..." instead of "ld: warning: dylib...", since I've seen a few cases without "ld: warning".

@zlscherr
Copy link

zlscherr commented Feb 3, 2021

comment:9

I was doing some testing today and could, for example, eliminate these warnings by doing something like:

MACOSX_DEPLOYMENT_TARGET=11.0 ./sage -t --random-seed=0 src/sage/docs/instancedoc.pyx

I think the underlying problem lies in the python that sage is using. You can see this, for example, by running sage and trying

import sysconfig
sysconfig.get_config_var("MACOSX_DEPLOYMENT_TARGET")

the issue is that the homebrew libraries were built on a particular operating system with its own deployment target, which is newer than the python target of sage. This problem also manifests itself in #31227 where /usr/bin/python3 has the target set to 10.14.6 for some reason.

I wonder if you can try to fix this by making sage set MACOSX_DEPLOYMENT_TARGET environment variable when it's run. You can probably get away with just setting it to whatever version of the Mac operating system you are on.

@jhpalmieri
Copy link
Member

comment:10

I like the idea but I'm not getting it to work:

% MACOSX_DEPLOYMENT_TARGET=11.2 ./sage
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 9.3.beta6, Release Date: 2021-01-17               │
│ Using Python 3.9.1. Type "help()" for help.                        │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: import sysconfig                                                                    
sage: sysconfig.get_config_var("MACOSX_DEPLOYMENT_TARGET")                                
'10.9'

Maybe part of the solution is #18272: we should not be artificially setting MACOSX_DEPLOYMENT_TARGET. That may only help when we are using Sage's own Python, but I think this problem also arises with /usr/bin/python3 or the homebrew Python 3.

@zlscherr
Copy link

zlscherr commented Feb 3, 2021

comment:11

What happen's if you try

MACOSX_DEPLOYMENT_TARGET=11.0 ./sage -t src/sage/misc/sagedoc.py

That won't change sysconfig's deployment target, but it might stop the linking warnings.

(I just ran make distclean for some reason and won't be able to test until tomorrow)

@zlscherr
Copy link

zlscherr commented Feb 3, 2021

comment:12

ok, that definitely didn't work. For some reason that seemed to work with /usr/bin/python3 but not with the sage built python which is built for target 10.9

@kiwifb
Copy link
Member

kiwifb commented Feb 3, 2021

comment:13

We should definitely stop arbitrarily defining MACOSX_DEPLOYMENT_TARGET, the main use for it is producing binaries that are backward compatible up to a point. We shouldn't have any business setting it to something else than the host building system value unless we make a binary build.

@jhpalmieri
Copy link
Member

comment:14

I've posted a branch at #18272 to stop defining MACOSX_DEPLOYMENT_TARGET. I've only tested it on Big Sur, but so far it's pretty good. I am still hitting these "ld" warning messages when building with /usr/bin/python3 (but not with homebrew or with Sage's own Python), so we will still need this ticket. I'm testing whether

% MACOSX_DEPLOYMENT_TARGET=11.0 make ptestlong

works when Sage is built with /usr/bin/python3 on top of #18272.

@jhpalmieri
Copy link
Member

comment:15

Marking as "needs info" because the suggestion from comment:9 may be the right approach, and it would be better than filtering out the warnings.

@jhpalmieri
Copy link
Member

comment:16

I like gh-zlscherr's suggestion, but what is the right way to do it?

  1. unconditionally (on OS X only, I mean) set MACOSX_DEPLOYMENT_TARGET before running doctests?
  2. set it conditionally based on some test of Python? If so, what test?
  3. don't set it at all, but print a message (either always or conditionally) suggesting that the user might set it?
  4. something else?

Do we set it in src/bin/sage-runtests or somewhere else?

@jhpalmieri
Copy link
Member

comment:18

Maybe we should filter out the warnings and on separate tickets deal with the underlying issue of setting MACOSX_DEPLOYMENT_TARGET. For example: I don't see the warnings at all on OS X 11.2 using homebrew's Python, but when Sage builds its own Python, I can't eliminate the warnings by setting MACOSX_DEPLOYMENT_TARGET during doctesting.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 13, 2021

comment:19

+1 for this strategy.

Let's get this ticket into Sage 9.3. https://github.com/mkoeppe/sage/actions/runs/649866485 tests this ticket together with #31492 - but there seems to be a scarcity of macOS runners on GH Actions at the moment

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 13, 2021

Author: John Palmieri

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 14, 2021

comment:21

This regexp does not seem to be working for me

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 14, 2021

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

288579dtrac 31204: suppress ld warning messages about wrong OS X version
e067c72trac 31204: fix regexp

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 14, 2021

Changed commit from 801c1a7 to e067c72

@jhpalmieri
Copy link
Member

comment:23

Try this one.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 14, 2021

comment:24

The example from comment:3 is still a problem

@jhpalmieri
Copy link
Member

comment:25

Replying to @mkoeppe:

The example from comment:3 is still a problem

It is, and the only thing I can think is to change the doctest:

diff --git a/src/sage/tests/cmdline.py b/src/sage/tests/cmdline.py
index 086704317f..82f825321b 100644
--- a/src/sage/tests/cmdline.py
+++ b/src/sage/tests/cmdline.py
@@ -292,15 +292,16 @@ def test_executable(args, input="", timeout=100.0, pydebug_ignore_warnings=False
         sage: (out, err, ret) = test_executable(["sage", fullname], pydebug_ignore_warnings=True)
         sage: print(out)
         499500
-        sage: err
-        'Compiling ...spyx...'
+        sage: import re
+        sage: bool(re.match('Compiling.*spyx.*', err))
+        True
         sage: ret
         0
         sage: (out, err, ret) = test_executable(["sage", name], cwd=dir, pydebug_ignore_warnings=True)
         sage: print(out)
         499500
-        sage: err
-        'Compiling ...spyx...'
+        sage: bool(re.match('Compiling.*spyx.*', err))
+        True
         sage: ret
         0
 

What do you think about that?

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 14, 2021

comment:26

Fine with me to change these doctests, could you push it to the ticket please?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 14, 2021

Changed commit from e067c72 to 55cf2ee

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 14, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

55cf2eetrac 31204: fix one doctest

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 14, 2021

comment:28

This works well.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 14, 2021

Reviewer: Matthias Koeppe

@jhpalmieri
Copy link
Member

comment:29

Good, thanks!

@vbraun
Copy link
Member

vbraun commented Mar 18, 2021

Changed branch from u/jhpalmieri/31204-ld-warnings to 55cf2ee

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

No branches or pull requests

5 participants