support current and future FreeBSD releases #985

Merged
merged 1 commit into from Jul 13, 2012

Projects

None yet

2 participants

@ivanov
Matplotlib Developers member

Also removed some unnecessary imports. This closes #225

This is an alternative to #982, and I consider this PR to be more future-proof. As I explain in the comments:

# basedir is a dictionary keyed by sys.platform, and on most platforms it is
# set to ['/usr/local', '/usr']. Giving this defaultdict a factory that
# returns this default removes the need to update this code every time a new
# version of freebsd comes out, for example, provided that the default basedir
# remains sufficient on that platform
basedir = defaultdict(lambda: ['/usr/local', '/usr'], {
    # execptions to the ['/usr/local', '/usr'] defaults
    ...

Implementation detail: I checked and defaultdict was introduced into collections at 2.5, so we're safe, since trunk no longer need to be 2.4 compatible.

@ivanov ivanov support current and future FreeBSD releases
Also removed some unnecessary imports. This closes #225
c28e8d7
@pelson
Matplotlib Developers member

I did consider this approach but I stuck to the explicit OS-es (safer bet) as I figured that, had the original author of setupext wanted to support every OS up-front they would have done so. That logic may not be sound as its likely that the number of supported OS-es has gradually increased rather than all of them being known up-front.

It is worth noting that this change in approach means that a user of AIX6 will now get paths that have been explicitly removed for AIX5 rather than a KeyError. I'm not sure which is worse :-) .

@ivanov
Matplotlib Developers member

I just dislike having a ton of copy-pasted lines for each FreeBSD release that is identical, so that our hand is otherwise forced to have to update this file on every new OS release. There are also lots of comments here about how we should not be using sys.platform in the first place. (One poster gives us a pass, but I think he refers to a change elsewhere in the mpl codebase, not the one we're talking about here).

As for AIX6, we'll fix that issue when (if) it gets reported ;)

For example, NetBSD was never included in the original basedir dictionary that was there, but they have a (presumably working) port that just patches up 'basedirlist' in the place it matters (they apparently disabled extensions, and did not need to patch it in make_extesnsion())

@pelson
Matplotlib Developers member

I just dislike having a ton of copy-pasted lines for each FreeBSD release

Agreed. But something like freebsd[0-9]+ as a regex could solve that problem (I'm not suggesting this as an improvement, merely as an alternative approach).

As for AIX6, we'll fix that issue when (if) it gets reported ;)

I don't have a problem with this approach, but to be completely explicit, this was not how setupext.py was originally authored (intentionally or not). Where before one would get an explosion when running setupext, one might now get strange usability issues because the wrong libraries were picked up.

For what its worth, this approach has my +1, but take that with a pinch of salt as I don't know the original motivation for limiting setupext to fixed OS-es in the first place.

@ivanov
Matplotlib Developers member

For what its worth, this approach has my +1, but take that with a pinch of salt as I don't know the original motivation for limiting setupext to fixed OS-es in the first place.

Yeah, this makes sense to me. From reading portions of Python Bug #12326, it seems like we 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
while at the same time dealing with the issues of future platform version bumps.

Let's give it another day before merging...

@pelson
Matplotlib Developers member

@ivanov: Since nobody has raised a concern with this approach, I suggest we merge this issue in 24hrs. I will close #982.

@ivanov
Matplotlib Developers member

sounds good, @pelson. There's 6 more hours before your proposed deadline, so I'll probably be asleep, but feel free to go ahead with the merge

@pelson pelson merged commit 97b3166 into matplotlib:master Jul 13, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment