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

support current and future FreeBSD releases #985

Merged
merged 1 commit into from Jul 13, 2012

Conversation

ivanov
Copy link
Member

@ivanov ivanov commented Jul 3, 2012

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.

Also removed some unnecessary imports. This closes matplotlib#225
@pelson
Copy link
Member

pelson commented Jul 3, 2012

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
Copy link
Member Author

ivanov commented Jul 3, 2012

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
Copy link
Member

pelson commented Jul 3, 2012

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
Copy link
Member Author

ivanov commented Jul 3, 2012

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
Copy link
Member

pelson commented Jul 12, 2012

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

@ivanov
Copy link
Member Author

ivanov commented Jul 13, 2012

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 added a commit that referenced this pull request Jul 13, 2012
Support current and future FreeBSD releases in the setupext.py build script.
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for FreeBSD >6.x
2 participants