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

pypy3: bump to 5.10.1, fix issues #1475

Closed
wants to merge 1 commit into from
Closed

Conversation

danchr
Copy link
Member

@danchr danchr commented Mar 24, 2018

  • address 'port lint' warning
  • properly suppress embedded dependencies

Closes: https://trac.macports.org/ticket/55679

Description

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 10.13.3
Xcode 9.2

Verification

Have you

  • checked your Portfile with port lint?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?

- address 'port lint' warning
- properly suppress embedded dependencies

Closes: https://trac.macports.org/ticket/55679
@danchr danchr added type: update by: member Created by a member with commit rights labels Mar 24, 2018
@danchr danchr requested a review from jmroot March 24, 2018 21:39
@macportsbot
Copy link

Notifying maintainers:
@jmroot for port pypy.

@macportsbot macportsbot added maintainer: open Affects an openmaintainer port type: bugfix labels Mar 24, 2018
@@ -35,7 +35,7 @@ patchfiles darwin.py.diff \
ffiplatform.py.diff

subport pypy3 {
revision 1
version 5.10.1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a port that has subports, especially when the subports have a different version than the main port, it's a good idea to keep a revision line in each subport, and in the main port's if {$subport eq $name} block, even if it's 0.

set default_prefix "/opt"
append default_prefix "/local"

reinplace "s|('/sw/', '${default_prefix}/')|('__PREFIX__',)|" \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of weird hacks, I would recommend handling this as we usually do, with a standard patchfile for extbuild.py that replaces the undesired prefixes with the string __PREFIX__. The reinplace below will then change that back to the desired prefix. In fact, the port used to do it that way, until you changed it in 238d27b. I would change it back.

@@ -143,6 +148,11 @@ destroot.args --builddir ${destroot}${prefix}/lib \
destroot.target package.py
destroot.post_args

subport pypy3 {
# we set destroot.args just above, so we cannot do this earlier
destroot.args-append --no-embedded-dependencies
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why the "above" code couldn't also append to destroot.args, rather than overwriting it?

If there is, then this untidiness might be avoided by moving the first subport pypy3 block below where destroot.args is set, so that all of the commands relating to the subport are in one place.

@@ -143,6 +148,11 @@ destroot.args --builddir ${destroot}${prefix}/lib \
destroot.target package.py
destroot.post_args

subport pypy3 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The subport procedure is intended to define a subport. This subport has already been defined above; it should not be redefined here. Instead, if this block must really be kept separate from the earlier subport pypy3 block, introduce this block with if {$subport eq "pypy3"} instead.

@@ -66,12 +65,18 @@ long_description \
instead of CPython is speed, as it runs generally faster.

post-patch {
reinplace "s+('/sw/', '/opt/local/')+('__PREFIX__',)+g" \
# the PyPy sources search for dependancies in /opt/local and /sw,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dependencies

if { ![catch {exec grep -lwre "/sw" -e "/opt/local" ${worksrcpath}}] } {
return -code error "didn't catch all references to /sw or /opt/local!"
if { ![catch {exec grep -lwre "/sw" -e ${default_prefix} ${worksrcpath}}] } {
return -code error "didn't catch all references to /sw or ${default_prefix}!"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this block really necessary? Have the developers really been making so frequent and sweeping changes to their code that this has actually been an issue? We have tons of other ports that use the patch-with-placeholder-then-reinplace strategy that haven't felt the need to add such sanity checks.

@macportsbot
Copy link

Travis Build #1788 Errored.

Lint results
--->  Verifying Portfile for pypy
--->  0 errors and 0 warnings found.

Port pypy success on xcode9.3beta. Log
Port pypy success on xcode8.3. Log
Port pypy success on xcode7.3. Log

The build timed out.

@pmetzger
Copy link
Member

@danchr Could you fix the things that @ryandesign pointed out above so we can merge this?

@pmetzger
Copy link
Member

pmetzger commented Apr 9, 2018

@danchr Any progress on fixing the issues that @ryandesign noted? It would be nice to commit this.

@pmetzger
Copy link
Member

@ryandesign, I think @danchr isn't going to show up again for a while. Do you want to add these fixes yourself so we can commit and close this, or should we just close this out until he returns?

@ryandesign
Copy link
Contributor

I don't have time to fix every problem. Literally anybody else with access could make the changes.

@pmetzger
Copy link
Member

Right, but others don't necessarily have time either, or at least, no one else has shown up with time (and I have fairly little myself).

It would be optimal if @danchr made them but he seems to have vanished for the moment. Do you mind if I close this for now? We can reopen it at will.

@danchr
Copy link
Member Author

danchr commented Apr 16, 2018

Sorry, but I just haven’t had the time lately! I should be able to take a look this weekend, but I can’t promise anything…

@pmetzger
Copy link
Member

@danchr If you don't have time, would you prefer to close this until you have time again?

@danchr
Copy link
Member Author

danchr commented Apr 16, 2018

Sure!

@danchr danchr closed this Apr 16, 2018
@danchr danchr deleted the fix-pypy3 branch July 25, 2018 11:33
@danchr danchr mentioned this pull request Jul 25, 2018
3 tasks
danchr added a commit that referenced this pull request Jul 29, 2018
* Bump to latest version, as mentioned.
* Address issues identified by Ryan Schmidt in the PR referenced
  below, such as too much cleverness with replacements.
* Properly suppress dependency embedding -- use ours instead.
* Reorganise the port to avoid duplication wherever possible.
* Add manual page.
* Add some comments.
* Use pypy2 for building pypy3.
* Prevent tkinter from using the system-provided Tk.

Closes: https://trac.macports.org/ticket/55679
Refs: #1475
danchr added a commit that referenced this pull request Jul 30, 2018
* Bump to latest version, as mentioned.
* Address issues identified by Ryan Schmidt in the PR referenced
  below, such as too much cleverness with replacements.
* Properly suppress dependency embedding -- use ours instead.
* Reorganise the port to avoid duplication wherever possible.
* Add manual page.
* Add some comments.
* Use pypy2 for building pypy3.
* Prevent tkinter from using the system-provided Tk.

Closes: https://trac.macports.org/ticket/55679
Refs: #1475
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
by: member Created by a member with commit rights maintainer: open Affects an openmaintainer port type: bugfix type: update
5 participants