Opened 5 years ago

Closed 22 months ago

#5873 closed defect (worksforme)

Fix matplotlib build on FreeBSD

Reported by: pjeremy Owned by: mabshoff
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: porting: BSD Keywords:
Cc: jason, stephen Merged in:
Authors: Reviewers: Stephen Montgomery-Smith, Paul Ivanov
Report Upstream: None of the above - read trac for reasoning. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by pjeremy)

0) Add support for FreeBSD later than 6.x

1) Explicitly add SAGE_LOCAL to the dependency search path for matplotlib for FreeBSD - superceded by #9202

2) gcc-4.3 on FreeBSD (though not the base gcc4.2) appears to define putchar() in <stdio.h> in a way that breaks the putchar() definitions inside ttconv. It's not immediately clear what the problem is (since there's no immediately obvious difference in the way putchar() is defined in <stdio.h>) so this patch takes the easy way out and undef's the offending putchar() macro - no longer needed with gcc-4.5

3) Individual character bounding boxes in AFM files do not have to be integral so convert each bounding box to a list of floats, rather than a list of ints. This corrects a problem where most of the tests would fail with "ValueError?: invalid literal for int() with base 10: '539.621'" on FreeBSD - now integrated into matplotlib

Attachments (3)

matplotlib-0.98.5.3rc0-svn6910.p3.patch (2.0 KB) - added by pjeremy 5 years ago.
5873.matplotlib.patch (818 bytes) - added by pjeremy 4 years ago.
matplotlib-0.99.1.p4.patch (585 bytes) - added by pjeremy 4 years ago.

Download all attachments as: .zip

Change History (22)

Changed 5 years ago by pjeremy

comment:1 Changed 5 years ago by jason

  • Summary changed from [with patch, needs review] Fix matplotlib build on FreeBSD to [with patch, needs work] Fix matplotlib build on FreeBSD

Of course, this should be added to the current matplotlib spkg.

Changed 4 years ago by pjeremy

comment:2 Changed 4 years ago by pjeremy

  • Authors set to Peter Jeremy
  • Cc jason added
  • Report Upstream set to N/A
  • Status changed from needs_work to needs_review
  • Summary changed from [with patch, needs work] Fix matplotlib build on FreeBSD to [patch] Fix matplotlib build on FreeBSD

5873.matplotlib.patch has been updated for matplotlib-0.99.1.p2. Of the patches mentioned in the original description, only the first part remains (and is still necessary).

The second part (related to putchar problems with gcc43) has been removed as it's no longer practical to build Sage with gcc43 on FreeBSD.

The third part (related to bounding box conversions) has been removed as an equivalent patch has been integrated into matplotlib-0.99.1

comment:3 follow-up: Changed 4 years ago by jason

The matplotlib spkg up at #9202 should take care of the remaining issue in this patch (by prepending the SAGE_LOCAL directory no matter what the platform). Can you check to see if #9202 fixes things?

comment:4 in reply to: ↑ 3 Changed 4 years ago by drkirkby

Replying to jason:

The matplotlib spkg up at #9202 should take care of the remaining issue in this patch (by prepending the SAGE_LOCAL directory no matter what the platform). Can you check to see if #9202 fixes things?

Any thoughts about this Peter? I noticed you created (or at least edited) a wiki page about the FreeBSD port, and still reference this old patch, which is probably no longer needed.

Dave

comment:5 follow-up: Changed 4 years ago by pjeremy

  • Status changed from needs_review to needs_work

Unfortunately, a variant of this patch is still needed to support FreeBSD later than FreeBSD6. Whilst #9202 means prepending SAGE_LOCAL should no longer be necessary, additional OS-related lines are still needed to support recent versions of FreeBSD.

comment:6 in reply to: ↑ 5 Changed 4 years ago by drkirkby

Replying to pjeremy:

Unfortunately, a variant of this patch is still needed to support FreeBSD later than FreeBSD6. Whilst #9202 means prepending SAGE_LOCAL should no longer be necessary, additional OS-related lines are still needed to support recent versions of FreeBSD.

If you wish to create one, I'll try to review it reasonably quickly. It makes review a lot easier if you can include things inside

#ifdef FREEBSD 
#endif

or if appropriate

#ifdef HAVE_BUGGY_GCC_ON_FREEBSD
#undef putchar
#endif

or similar. Otherwise, it requires the reviewer to have a much deeper knowledge of the code to evaluate if the changes are desirable or not. If it can be seen the changes only affect FreeBSD, then it will be much easier to get a positive review. That's been my experience with Solaris and OpenSolaris related problems.

Dave

Changed 4 years ago by pjeremy

comment:7 follow-up: Changed 4 years ago by pjeremy

  • Description modified (diff)
  • Report Upstream changed from N/A to Completely fixed; Fix reported upstream

Point 0 has been reported upstream as https://sourceforge.net/tracker/?func=detail&aid=3031051&group_id=80706&atid=560722 and an updated patch (not yet converted to spkg) uploaded.

comment:8 in reply to: ↑ 7 Changed 3 years ago by kcrisman

Replying to pjeremy:

Point 0 has been reported upstream as https://sourceforge.net/tracker/?func=detail&aid=3031051&group_id=80706&atid=560722 and an updated patch (not yet converted to spkg) uploaded.

Unfortunately, this link no longer works, as matplotlib has moved its bug tracker to Github. Pleasantly, the ticket is still there. Sadly, the patch appears to have been lost there, though it's still here.

comment:9 Changed 2 years ago by kcrisman

Apparently Stephen Montgomery-Smith has gotten matplotlib to build fine for Sage in the meantime, or possibly using a system matplotlib.

comment:10 Changed 22 months ago by kcrisman

  • Status changed from needs_work to needs_info

More success with this thread. Checking whether system variant or Sage version.

comment:11 Changed 22 months ago by kcrisman

  • Authors Peter Jeremy deleted
  • Milestone changed from sage-5.1 to sage-duplicate/invalid/wontfix
  • Reviewers set to Stephen Montgomery-Smith
  • Status changed from needs_info to needs_review

Some of the reason this is unnecessary is probably due to the upgrades in gcc.

I don't understand, though, why the patch for newer FreeBSD is no longer necessary. The current mpl source (June 2012) does not have it incorporated.

This is weird enough that I'm not putting positive review; it seems like there should be a key error at this spot if we don't have something in the dictionary for this system.

comment:12 Changed 22 months ago by pi

I just wanted to chime in with the reason that the patch for newer FreeBSD was not necessary on FreeBSD itself, is because their ports tree incorporates it itself downstream. So that's the reason it is not in the current MPL source (we haven't heard enough people building it natively complain, they were probably using the system variant).

As it stands, either MPL PR #982 or MPL PR #985 will make this a non-issue (and you can grab either of those patches in the meantime, they will both also work for FreeBSD10, and MPL #985 will work for any other future releases, as well any other POSIX systems that got left out.

comment:13 Changed 22 months ago by kcrisman

  • Cc stephen added

Thanks for this info. Strange, though, because the testing in question was not using the system mpl, as far as I know.

Stephen, any thoughts on this? We *could* make a custom spkg with one of these patches.

comment:14 Changed 22 months ago by stephen

I am going to have to say that I don't know why the FreeBSD port of sage doesn't build without this patch. I looked at the FreeBSD port of matplotlib, and this patch was included there. I just don't understand what is going on. Let me look into it some more.

comment:15 Changed 22 months ago by pi

TLDR: you don't need this patch.

The mpl setupext.py code in question gets used when there is not an entry for basedirlist in setup.cfg, in which case it'll grab it from this pre-defined basedir dictionary using sys.platform as they key. From what I see that's checked into hg for SAGE, you do define basedirlist in setup.cfg, which is why this patch is un-necessary. The code is in the SPKG matplotlib.../make-setup-config.py:7

config.set('directories', 'basedirlist', os.environ['SAGE_LOCAL'])

which is why this patch is not needed for SPKG matplotlib being built on any platform.

From what I understand from the discussion at Python Bug #12326, it seems like we (MPL) should not have been using sys.platform in the first place for making these decisions (but given the somewhat exotic nature of the platforms which have exceptions there now, it's best to remain conservative about re-writing that portion of the code to use something like the platform modules). But to reiterate, the approach taken in the SPKG bypasses this fragility for SAGE that is completely platform independent.

comment:16 Changed 22 months ago by kcrisman

  • Report Upstream changed from Completely fixed; Fix reported upstream to None of the above - read trac for reasoning.
  • Reviewers changed from Stephen Montgomery-Smith to Stephen Montgomery-Smith, Paul Ivanov

@pi: So it sounds like between the practical experience of stephen and the upstream and our config you are pointing out, that we don't have to do anything. Even though eventually mpl will have some other workaround.

If that's correct, I've put what I think is your real name here - just switch to "positive review"! I'm switching the upstream to "none of the above" since this is a somewhat unusual situation. Is this your first contribution? If so, welcome to the Sage team!

Last edited 22 months ago by kcrisman (previous) (diff)

comment:17 Changed 22 months ago by pi

  • Status changed from needs_review to positive_review

@kcrisman it is my first direct Sage contribution, thanks!

comment:18 Changed 22 months ago by kcrisman

  • Summary changed from [patch] Fix matplotlib build on FreeBSD to Fix matplotlib build on FreeBSD

Great!

comment:19 Changed 22 months ago by jdemeyer

  • Resolution set to worksforme
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.