I'll give @ale the two-week timeout for maintainers. We still have time to next quarterly branch.

@mandree The text has been already reduced. Would you like to have it removed completely?

Incorporate Baptiste's proposal

In D45073#1028240, @jilles wrote:

The immediately preceding sentence about shell functions is an example of unexpected behaviour of set -e. The part "all commands of the function are considered to be tested as well" may result in code unexpectedly executing in the function and the function returning a different return status.

Many more examples can be found at https://mywiki.wooledge.org/BashFAQ/105 . Bash does not work exactly the same as our sh, but most of the problems are the same.

Bash's behaviour related to set -e has changed over the versions, but it has not succeeded in fixing the confusing parts. I don't believe it's possible to fix, and it is best to leave it unchanged so it is at least consistent.

set -e works reasonably well for scripts that are plain lists of commands with possible case, for, if, while and until, and only use && and || in conditions, which probably explains its use in make. However, even then, command substitution might lead to confusing results, for example because expr returns status 1 if the result is zero or the empty string, and the exit status from the inner command is passed through only in particular cases (for the last command substitution if there is no utility name).

A linting tool will probably be more effective (I've heard of ShellCheck, but I don't know whether it can catch missing error checks). With a linting tool, it matters less how beautiful the code looks but whether the tool can analyze it. Not enabling set -e makes the code easier to analyze, for example because functions don't behave differently depending on whether their return status is tested.

Another option is to use a better programming language. For a long time, there did not use to be one in the base system, but perhaps Lua is now a suitable option.

Followup review created: https://reviews.freebsd.org/D45073

So I merged now the explicit exits. We have two months until next quarterly for failures. As @mandree said, enough time. I will come up with a review for the manpage as well.

No response fron @ale , I will go ahead and push this with @jrm's approval.

Bump port revision

@ale, you want to take a look at?

Stupid question: Should I bump the port revision here?

Lev approached me via e-mail whether I'd like to take ownership of it. I have agreed. He shall approve this review.

Improved version which also supports OpenSSL 1.1.1 by reverting a few changes
from Tomcat Native. Ran full Tomcat test suites, all tests pass.

In D44824#1021968, @jrm wrote:

Thanks. Looks good. Could you also run it through something like portlint -AC and portclippy/portfmt?

One important thing to note: I could bring back OpenSSL 1.1.1 support with a simple patch because

• https://bz.apache.org/bugzilla/show_bug.cgi?id=66667 has not been solved
• It is almost only:

In D44822#1021925, @otis wrote:

I'd also suggest to bump PORTREVISION to also reflect the future binary packages (this might not be necessary if you plan to do a quick update).

Bump port revision

Ale's response: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=276736#c12

So which route should I go @mandree? set or exit?

In D42719#1021272, @jrm wrote:

To elaborate, I felt that specifically checking for the issue at hand is unlikely to introduce any new problems, but adding set -e, which, of course, affects the whole script is more likely. That said, since @michael is prepared to take on any new problems, I'm fine either way.

Use exit explicitly

Tried || exit $? locally, same result:

If the || exit $? version is preferred I will try it, no issue.

Rebased and tested again with poudriere-built packages:

In D44694#1019044, @jrm wrote:

LGTM However, someone from portmgr should approve. I'll try to get someone to look today.

Remove trailing period

Any other opinions?

In D44356#1011926, @jrm wrote:

@cperciva, it would be good to get your approval here as well.

@vvd, do you want to update this review?

Removed inline patch in favor of remote one.

fetch patch from remote

In D44338#1011233, @ale wrote:

I think you should change the WWW entry to point to 1.3

apply comments from ale@

This is now a minimal change, please have a look guys.

In D44229#1009025, @jrm wrote:

FYI, devel/eclipse failed in my 15.0-CURRENT 1500008 amd64 jail.

http://pkg.ftfl.ca/data/15amd64-default/2024-03-05_13h23m40s/logs/errors/eclipse-4.24_2.log

It doesn't seem like this change should have any effect, but can you verify this?

Include java/eclipse

In D44229#1008960, @vvd wrote:

I think you can add java/eclipse here too.
Probably https://reviews.freebsd.org/D44153 isn't actual anymore.

FTR: https://reviews.freebsd.org/D44229

Address improvement proposal from @emaste.

I will take care of these: