Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#65721 closed enhancement (fixed)

app portgroup: create new 1.1 version, utilizing callback mechanism

Reported by: mascguy (Christopher Nielsen) Owned by: mascguy (Christopher Nielsen)
Priority: Normal Milestone:
Component: ports Version: 2.7.2
Keywords: portgroup Cc: ctreleaven (Craig Treleaven)
Port: gramps

Description (last modified by mascguy (Christopher Nielsen))

In cases where other portgroups are also used - such as python - this can result in issues. For example, it's currently necessary to place the inclusion for this pg at the tail end of port gramps, otherwise post-destroot handlers are run in the wrong order.

Create a new version, utilizing a callback, to eliminate these issues.

Change History (24)

comment:1 Changed 2 years ago by mascguy (Christopher Nielsen)

Port: gramps added

comment:2 Changed 2 years ago by jmroot (Joshua Root)

There is never any guarantee what order multiple pre- or post- phase blocks will run in, so anything that relies on them running in a particular order is broken. If gramps needs something to be done before some post-destroot block runs, it needs to do it somewhere that runs earlier like destroot or pre-destroot.

comment:3 in reply to:  2 ; Changed 2 years ago by mascguy (Christopher Nielsen)

Replying to jmroot:

There is never any guarantee what order multiple pre- or post- phase blocks will run in, so anything that relies on them running in a particular order is broken. If gramps needs something to be done before some post-destroot block runs, it needs to do it somewhere that runs earlier like destroot or pre-destroot.

Hmmm, I thought that post-destroot blocks are run in a deterministic manner, based on the order in which they're registered.

So that's not the case?

comment:4 in reply to:  3 Changed 2 years ago by mascguy (Christopher Nielsen)

Replying to mascguy:

Hmmm, I thought that post-destroot blocks are run in a deterministic manner, based on the order in which they're registered.

To keep this on track: The goal is to improve the situation, though it still may not be perfect even with the callback mechanism. (As with other pg interactions, inclusion order can still occasionally be problematic.)

Does that make sense? Your thoughts?

comment:5 Changed 2 years ago by mascguy (Christopher Nielsen)

On a separate note: Presently the app pg checks for the executable in a pre-destroot block, which can potentially be premature. I'd like to move this check to post-destroot:

pre-destroot {
    if {[tbool app.create]} {
        # Ensure app.name is valid.
        if {[regexp {[/]} ${app.name}]} {
            return -code error "app.name ${app.name} contains illegal characters"
        }

        # Make the app bundle directories.
        xinstall -d ${destroot}${applications_dir}/${app.name}.app/Contents/MacOS \
                    ${destroot}${applications_dir}/${app.name}.app/Contents/Resources
    }
}

We can continue to create the app bundle directories in the pre-destroot block. But to avoid issues, we shouldn't check for executable existence until post-destroot.

Thoughts?

comment:6 in reply to:  5 Changed 2 years ago by mascguy (Christopher Nielsen)

Replying to mascguy:

On a separate note: Presently the app pg checks for the executable in a pre-destroot block, which can potentially be premature. I'd like to move this check to post-destroot

Duh, that's only validating that the name doesn't continue illegal characters, not whether it exists. Never Mind! Lol

comment:7 in reply to:  3 ; Changed 2 years ago by ryandesign (Ryan Carsten Schmidt)

Replying to mascguy:

Replying to jmroot:

There is never any guarantee what order multiple pre- or post- phase blocks will run in, so anything that relies on them running in a particular order is broken.

Hmmm, I thought that post-destroot blocks are run in a deterministic manner, based on the order in which they're registered.

As far as I have been able to tell by observation, they run in the order registered. I'm not aware of any reason why we should change that behavior in base; changing that would just break ports that rely on a particular order. Although it is probably best for ports not to rely on the order, some ports probably do.

Replying to jmroot:

If gramps needs something to be done before some post-destroot block runs, it needs to do it somewhere that runs earlier like destroot or pre-destroot.

It's not gramps that need it. The app portgroup contains a sanity check that verifies that the executable you've specified exists. If the gramps portfile had created the executable there would be no problem, but in this case the executable is a symlink that is created by the python portgroup. The app portgroup's sanity check cannot be moved any earlier than post-destroot because it checks things that the port should create at destroot time, and the python portgroup's symlink creation code cannot be moved any earlier than post-destroot because it creates symlinks to every binary that was created by the port at destroot time.

The problem occurs because the app portgroup's post-destroot block is registered the moment the app portgroup is included, while the python portgroup's post-destroot block is registered the moment python.versions is set. Therefore, since portgroups are typically included up top and python.versions isn't typically set until later, the app portgroup's post-destroot block runs before the python portgroup's post-destroot block.

I suggested that the solution to this issue is to have the gramps portfile tell the app portgroup the location of the real executable that already exists, not the location of the symlink to it that the python portgroup eventually creates.

I don't think changing the order in which the app portgroup's post-destroot block runs is a good idea at such a late time (so long after the app portgroup was created and deployed). All of the ports that already use the app portgroup work with the way the portgroup is currently designed. Changing it now (e.g. by using an end-of-portfile callback that registers the post-destroot at a different time) has the potential to break existing ports.

comment:8 in reply to:  7 Changed 2 years ago by mascguy (Christopher Nielsen)

Replying to ryandesign:

I don't think changing the order in which the app portgroup's post-destroot block runs is a good idea at such a late time (so long after the app portgroup was created and deployed). All of the ports that already use the app portgroup work with the way the portgroup is currently designed. Changing it now (e.g. by using an end-of-portfile callback that registers the post-destroot at a different time) has the potential to break existing ports.

Agreed, that was my big concern as well. And while we could theoretically add a new pg option to enable that behavior - disabled by default - it's safer not to touch it.

That said, we could create a new 1.1 version of the pg... with minimal refactoring for a callback. What do you folks think?

comment:9 Changed 2 years ago by kencu (Ken)

I think you might try just using the app portgroup's launch script option instead for this?

It doesn't care when the symlink may or may not be made, and usually works well.

comment:10 in reply to:  9 ; Changed 2 years ago by mascguy (Christopher Nielsen)

Replying to kencu:

I think you might try just using the app portgroup's launch script option instead for this?

Yep, that's another option.

Ditto for a few other alternatives:

  • Segregate app bundle into a separate port. But shouldn't be necessary, when we simply want to add an app bundle to an existing port.
  • Set app.executable to the real binary, per Ryan's previous suggestion. But this option is less simple than it could be, as it has to be done within a phase. (Simply because python.bin isn't set correctly until later.) For example:
post-build {
    # Can't do this outside of a phase, as `python.bin` isn't set to selected Python version until later
    app.executable ${python.bin}/${name}
}

All of which further complicate what should otherwise "just work."

comment:11 in reply to:  10 ; Changed 2 years ago by kencu (Ken)

Replying to mascguy:

Replying to kencu:

I think you might try just using the app portgroup's launch script option instead for this?

Yep, that's another option.

Yes, I added that option some years ago, for just these kinds of issues. It is a disarmingly simple way of getting past these hiccups. It also has the very helpful feature of properly setting up the PATH, which the app PG method does not do by default.

I considered making it the default option, as it just seems to be the right thing to do usually, but left it as there were so many ports already using the existing method, and I didn't want to have to rebuild and try them all.

It usually works, but like anything, not guaranteed. Has to be tested.

comment:12 in reply to:  11 Changed 2 years ago by mascguy (Christopher Nielsen)

Replying to kencu:

Yes, I added that option some years ago, for just these kinds of issues. It is a disarmingly simple way of getting past these hiccups. It also has the very helpful feature of properly setting up the PATH, which the app PG method does not do by default.

It might be preferable to avoid use of launch scripts, unless absolutely necessary. Particularly given the complexities surrounding code-signing, GateKeeper security, etc.

Per issue:65302

comment:13 Changed 2 years ago by mascguy (Christopher Nielsen)

Unless someone can provide a strong argument against it, I'm going to add an app 1.1 pg using a callback. Folks are welcome to continue using app 1.0, and no further debate is necessary.

However, I'll be migrating my work to the new version though, as this has gotten a bit silly.

comment:14 Changed 2 years ago by mascguy (Christopher Nielsen)

Description: modified (diff)
Summary: app portgroup: callback not used, causing issues when combined with other pgsapp portgroup: create new 1.1 version, utilizing callback mechanism

comment:15 Changed 2 years ago by ctreleaven (Craig Treleaven)

Cc: ctreleaven added

comment:16 in reply to:  13 Changed 2 years ago by mascguy (Christopher Nielsen)

Replying to mascguy:

Unless someone can provide a strong argument against it, I'm going to add an app 1.1 pg using a callback.

For anyone interested in my approach, this is what I'm thinking:

  • Refactor minimally, to ensure it's as close to app 1.0 as possible. Also reduces chance of introducing any bugs.
  • Include a backward-compatibility option, which disables use of callback.
    • Provides a migration path for existing ports.
    • Would also allow us to ultimately fold the changes back into app 1.0, avoiding the need for a new version of the pg.
    • But if we do fold back into app 1.0, that option would be enabled by default.
  • Implement some type of unit-testing approach, to automate validation of error cases. Not strictly necessary, but would be nice!

comment:17 Changed 2 years ago by mascguy (Christopher Nielsen)

Resolution: wontfix
Status: assignedclosed

Originally, I never considered solving this rather rare situation, via a callback within the port itself. And while such an approach isn't quite as simple as a true callback-enabled pg, it's clean enough that I'm fine with it.

So let's table this for now. We can always revisit down the road, if there's a desire to do it.

comment:18 Changed 2 years ago by mascguy (Christopher Nielsen)

Type: defectenhancement

comment:19 in reply to:  17 Changed 2 years ago by mascguy (Christopher Nielsen)

Resolution: wontfix
Status: closedreopened

Replying to mascguy:

Originally, I never considered solving this rather rare situation, via a callback within the port itself. And while such an approach isn't quite as simple as a true callback-enabled pg, it's clean enough that I'm fine with it.

As more seasoned folks probably already know, this approach causes issues too.

So let's move forward with the new pg. It's tested and ready for review/feedback, so I'll be submitting a PR shortly.

comment:20 Changed 2 years ago by mascguy (Christopher Nielsen)

comment:21 Changed 2 years ago by Christopher Nielsen <mascguy@…>

Resolution: fixed
Status: reopenedclosed

In 723048be8dd46a8f0a15ad5261445b06fb45deb3/macports-ports (master):

pg app 1.1: new version, utilizing callback mechanism
Fixes: #65721

comment:22 in reply to:  10 ; Changed 2 years ago by jmroot (Joshua Root)

Replying to mascguy:

  • Set app.executable to the real binary, per Ryan's previous suggestion. But this option is less simple than it could be, as it has to be done within a phase. (Simply because python.bin isn't set correctly until later.) For example:
post-build {
    # Can't do this outside of a phase, as `python.bin` isn't set to selected Python version until later
    app.executable ${python.bin}/${name}
}

Why can't app.executable be set right after python.default_version?

BTW, it would have been helpful to reference #65716 in these laters tickets and PRs, so someone who came in late could know what the actual problem prompting all this was.

comment:23 in reply to:  22 Changed 2 years ago by mascguy (Christopher Nielsen)

Replying to jmroot:

Why can't app.executable be set right after python.default_version?

Do you mean within proc py_setup? Or outside of that, after variant processing - and a call to that proc - has been made?

Version 0, edited 2 years ago by mascguy (Christopher Nielsen) (next)

comment:24 Changed 2 years ago by jmroot (Joshua Root)

Has to be inside the variants the way it's currently set up, since the version isn't set before that. I would tend to do something like this instead when a value set based on a variant is needed before variants are executed:

variant python37 conflicts python38 python39 description {Use Python 3.7} {}
variant python38 conflicts python37 python39 description {Use Python 3.8} {}
variant python39 conflicts python37 python38 description {Use Python 3.9} {}
foreach py_ver {37 38 39} {
    if {[variant_isset python${py_ver}]} {
        python.default_version \
                        ${py_ver}
    }
}
depends_lib-append \
                        port:py${python.version}-bsddb3 \
                        port:py${python.version}-gobject3 \
                        port:py${python.version}-Pillow \
                        port:py${python.version}-pyicu

app.executable ${python.prefix}/bin/${name}
Note: See TracTickets for help on using tickets.