app portgroup: create new 1.1 version, utilizing callback mechanism

[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.

[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.

[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?

[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?

[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?

[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

[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.

[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?

[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.

[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."

[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.

[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

[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.

[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!

[mascguy (Christopher Nielsen)]

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.

[mascguy (Christopher Nielsen)]

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.

[mascguy (Christopher Nielsen)]

PR created:

​16148 - pg app 1.1: new version, utilizing callback mechanism

[Christopher Nielsen <mascguy@…>]

In 723048be8dd46a8f0a15ad5261445b06fb45deb3/macports-ports (master):

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

[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.

[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 the python variant declarations?

[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}