Opened 12 years ago

Closed 9 years ago

#37048 closed defect (fixed)

weechat: python variants

Reported by: ryandesign (Ryan Carsten Schmidt) Owned by: harciga
Priority: Normal Milestone:
Component: ports Version:
Keywords: maintainer haspatch Cc: kurthindenburg (Kurt Hindenburg), Ionic (Mihai Moldovan)
Port: weechat

Description

The weechat port has a python variant:

variant python description {Bindings for python plugins} {
    configure.args-delete   --disable-python
    configure.args-append   --enable-python
    depends_lib-append      path:bin/python:python26
}

The dependency on path:bin/python:python26 is incorrect because the python26 port does not provide /opt/local/bin/python; it provides /opt/local/bin/python2.6. You should offer variants for each supported version of python (e.g. python25, python26, python27) and in each one depend on the correct python port and use configure args, environment variables or patches to communicate to the weechat build system which version of python should be used. The python variant should be kept around for one year, turned into a stub variant, depending on the python26 variant, to help users upgrade. ("variant python requires python26 description {Legacy compatibility variant} {}")

Or if you're feeling adventurous, and even better solution would be to offer the python bindings as subports instead of as variants. Not sure it the weechat build system will easily accommodate that however.

Attachments (6)

Portfile-weechat.diff (2.7 KB) - added by harciga 9 years ago.
Portfile-weechat.2.diff (2.7 KB) - added by harciga 9 years ago.
Portfile-weechat.3.diff (2.8 KB) - added by harciga 9 years ago.
weechat-python.patch (2.1 KB) - added by larryv (Lawrence Velázquez) 9 years ago.
adding variants without eval+subst
Portfile-weechat.4.diff (3.7 KB) - added by harciga 9 years ago.
Portfile-weechat.5.diff (2.8 KB) - added by harciga 9 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 11 years ago by neverpanic (Clemens Lang)

Owner: changed from nefar@… to starkhalo@…

Re-assigning to new maintainer.

Changed 9 years ago by harciga

Attachment: Portfile-weechat.diff added

comment:2 Changed 9 years ago by kurthindenburg (Kurt Hindenburg)

Cc: khindenburg@… added

Cc Me!

comment:3 Changed 9 years ago by kurthindenburg (Kurt Hindenburg)

starkhalo, do the variants build OK for you? I'm getting errors on +python27

clang: error: no such file or directory: 'Python.framework/Versions/2.7/Python'
make[2]: *** [src/plugins/python/python.so] Error 2

comment:4 in reply to:  3 Changed 9 years ago by harciga

Sure they did, before the cmake change.

 cmake.out_of_source yes

Guess I'll fix the $patchfile later, I have to see in the PortGroup cmake what that does to ${worksrcpath}

Cheers.

starkhalo, do the variants build OK for you? I'm getting errors on +python27

clang: error: no such file or directory: 'Python.framework/Versions/2.7/Python'
make[2]: *** [src/plugins/python/python.so] Error 2

comment:5 in reply to:  3 ; Changed 9 years ago by harciga

This appears to do the trick

diff --git a/ongoing b/Portfile
index 7336489..50095ef 100644
--- a/Portfile.orig
+++ b/Portfile
@@ -95,7 +95,7 @@ foreach s ${pythons_suffixes} {
 
 # TODO: Ideally this should go inside its corresponding python variant.
 post-configure {
-    set patchfile ${worksrcpath}/src/plugins/python/CMakeFiles/python.dir/link.txt
+    set patchfile ${worksrcpath}/../build/src/plugins/python/CMakeFiles/python.dir/link.txt
 
     if {[file exists ${patchfile}]} {
         reinplace -E "s| \(Python.framework\)| ${frameworks_dir}/\\1|g" ${patchfile}

I'll verify later tonight with the subport and py34/py35

comment:6 in reply to:  5 ; Changed 9 years ago by larryv (Lawrence Velázquez)

Do not assume that the CMake build directory is “$worksrcpath/../build”. Use “${cmake.build_dir}”.

comment:7 in reply to:  6 ; Changed 9 years ago by larryv (Lawrence Velázquez)

Actually, that would assume cmake.out_of_source yes. You should use ${configure.dir}.

comment:8 in reply to:  7 Changed 9 years ago by harciga

Replying to larryv@…:

Actually, that would assume cmake.out_of_source yes. You should use ${configure.dir}.

Thanks, I'll update the ${patchfile} with ${configure.dir} and verify that it all builds fine later tonight.

Changed 9 years ago by harciga

Attachment: Portfile-weechat.2.diff added

comment:9 Changed 9 years ago by Ionic (Mihai Moldovan)

Cc: ionic@… added

Two nits:

  1. For python3X, you never delete -DENABLE_PYTHON=OFF. Given that -DENABLE_PYTHON=ON is added afterwards, let's hope cmake always uses the last seen option to override previous ones? For python2X, -DENABLE_PYTHON=ON is passed two times (should cause no harm functionality-wise.)
  1. depends_lib-append path:bin/python:${p} should be depends_lib-append port:${p}. bin/python is not provided by any port but set via port select (if the user even did that - there's enough installations with no "default" python binary selected) and you have no way of telling whether this binary corresponds to the selected python variant because you don't know the version.

Changed 9 years ago by harciga

Attachment: Portfile-weechat.3.diff added

comment:10 Changed 9 years ago by Ionic (Mihai Moldovan)

I've said this on IRC before, but you probably missed it:

You don't want to check if the last digit/character is a three, but the first one. So the comment should be updated and the regex changed from (3$) to (^3).

Everything else looks good to me.

comment:11 Changed 9 years ago by larryv (Lawrence Velázquez)

If you’re going to go with the regex, this…

set t ""

…is unnecessary. You just need this.

regexp {(^3)?} $s t

If s does not begin with “3”, then t will be empty.

Changed 9 years ago by larryv (Lawrence Velázquez)

Attachment: weechat-python.patch added

adding variants without eval+subst

comment:12 Changed 9 years ago by larryv (Lawrence Velázquez)

To be honest, I don’t think there’s any need to dynamically generate the variants like this. There shouldn’t be more than 4 Python variants ever (by policy), and it makes the portfile much harder for the rest of us to understand. The patch I just added demonstrates how most of the common logic can be abstracted out in a different way. (I haven’t tested it.)

Changed 9 years ago by harciga

Attachment: Portfile-weechat.4.diff added

comment:13 Changed 9 years ago by Ionic (Mihai Moldovan)

Please use notes instead of ui_warn.

Are you sure you don't want to handle common variants code like larry suggested?

comment:14 Changed 9 years ago by larryv (Lawrence Velázquez)

You’re duplicating a lot of code here. I strongly suggest doing it my way.

Also drop the 3.5 variant. We aren’t adding any 3.5 stuff until the stable release.

Changed 9 years ago by harciga

Attachment: Portfile-weechat.5.diff added

comment:15 in reply to:  13 Changed 9 years ago by harciga

Replying to ionic@…:

Please use notes instead of ui_warn.

Are you sure you don't want to handle common variants code like larry suggested?

Replaced ui_warn with `notes. No.

comment:16 in reply to:  14 Changed 9 years ago by harciga

Replying to larryv@…:

You’re duplicating a lot of code here. I strongly suggest doing it my way.

Also drop the 3.5 variant. We aren’t adding any 3.5 stuff until the stable release.

Dropped python35 until further notice.

comment:17 Changed 9 years ago by Ionic (Mihai Moldovan)

Ok.

No, you aren't sure, or no, you won't change it? I'm fine with either, if you want me to commit it, please say it.

comment:18 in reply to:  17 Changed 9 years ago by harciga

Replying to ionic@…:

Ok.

No, you aren't sure, or no, you won't change it? I'm fine with either, if you want me to commit it, please say it.

I don't see the need to change it again, the version I rewrote is clearer and after removing the python35 variant, just a few lines longer.

Looks to me it's just a question of style at this point, and since it works fine either way, just commit it as is.

Please.

comment:19 Changed 9 years ago by Ionic (Mihai Moldovan)

Keywords: maintainer haspatch added
Resolution: fixed
Status: newclosed
Version: 2.1.2

Committed as r138277. Thanks!

Note: See TracTickets for help on using tickets.