[JENKINS-67911] Stapler does not compile with Java 11

[basil]

Problem

Compiling Stapler with Java 11 and <release>11</release> yields the following results:

[ERROR] jenkinsci/stapler/groovy/src/main/java/org/kohsuke/stapler/jelly/groovy/StaplerClosureScript.java:[30,18] error: as of release 9, '_' is a keyword, and may not be used as an identifier
[ERROR] jenkinsci/stapler/groovy/src/main/java/org/kohsuke/stapler/jelly/groovy/StaplerClosureScript.java:[31,15] error: as of release 9, '_' is a keyword, and may not be used as an identifier
[ERROR] jenkinsci/stapler/groovy/src/main/java/org/kohsuke/stapler/jelly/groovy/StaplerClosureScript.java:[38,18] error: as of release 9, '_' is a keyword, and may not be used as an identifier

Solution

Rename the two _ functions as gettext, which is a valid Java 9+ identifier.

For backward compatibility with scripts like this, define _() as a metamethod that delegates to gettext().

Testing done

Set a breakpoint in gettext() and verified I could hit it from the Configure System page after this PR just as I could hit the old _() break point from that page before this PR.

Also compiled Stapler successfully with Java 11 and <release>11</release>.

Fixes #309

[daniel-beck]

What is the intended way forward for view definitions? Are they supposed to call gettext("foo") instead of _("foo") after this change?

If so, do we expect developers to know about gettext to make the mnemonic connection, or would another name be better?

[basil]

What is the intended way forward for view definitions? Are they supposed to call gettext("foo") instead of _("foo") after this change?

I could go either way on this one. No strong preference.

[jglick]

I would not expect Groovy script authors to know about the new name.

[jglick]

Is there already text coverage for this function in-repo? I guess something in jenkins would exercise it anyway.

[basil]

Not that I can see, which is why I tested it manually.

[jglick]

I am looking into making a unit test for this, hold on a bit…

[jglick]

Can merge #344 into this, or merge that first and then merge in master to verify, etc.

[basil]

merge that first and then merge in master to verify, etc.

Done

[basil]

Are they supposed to call gettext("foo") instead of _("foo") after this change?

I thought about this some more and realized that maybe it is better to migrate callers over to avoid the risk that someday _() will stop working in Groovy, as well. Certainly the metaclass approach is more complex, so it would be nice if that complexity was not needed for the common case.

If so, do we expect developers to know about gettext to make the mnemonic connection, or would another name be better?

I have opined in the past that I do not find code review feedback about naming to be all that interesting. It is very often a subjective matter. But also the style of this question is just not very conducive to the way my brain works. I have not intentionally been ignoring the question, but I find it difficult to have such conversations. The best way to get me to take action in a code review is to tell me that you don't like something, to explain the reason why you don't like it, and (ideally) to suggest a concrete solution or at least a concrete action item that I can take in order to drive resolution of the issue. This type of open-ended question- and discussion- focused code review is really difficult for me to navigate, as there is a lot of implicit communication (which is difficult for me).

For example, when I read the question "would another name be better?" I can clearly tell that you have some sort of issue with the name, but it isn't clear to me what that issue is or how I could make you feel better about it. Whereas if you wrote something like: "I don't like the name A for reason X, and I think name B would be better for reason Y" then I would have something concrete to go on.

[daniel-beck]

@basil Thanks for your response.

The name itself does not indicate a relationship to localization to me. I checked whether gettext is something existing elsewhere (IIRC you previously reused a Unix name for something else, but don't remember what, so that triggered that), and now I'm aware of it, it makes sense; but lacking that awareness, I was confused.

Now you may decide it's well-known enough, and/or that I'm an outlier with not seeing a relationship to localization, that's fine. Or that any name is better than _ (or % for that matter). But I think it is important to explicitly make such decisions, i.e. while considering the potential downsides, rather than implicitly, perhaps due to a potential lack of awareness, which my question was trying to get at.

(Besides the potential confusing around the purpose, it's also pretty close to text, which is used for something else.)

[basil]

Ah I see. When I first read this code, it seemed clear to me that Kohsuke's use of _() was clearly borrowed from the traditional C/gettext idiom. I don't know the history of this idiom, but the use of _() is common in C codebases, with GNU gettext as the standard implementation on modern Linux systems. This convention has spread into other languages with a C heritage, like Python. See for example Django:

Specify a translation string by using the function gettext(). It’s convention to import this as a shorter alias, _, to save typing. Python's standard library gettext module installs _() into the global namespace, as an alias for gettext().

And from the gettext docs:

Many packages use _ (a simple underline) as a keyword, and write _("Translatable string") instead of gettext ("Translatable string"). Further, the coding rule, from GNU standards, wanting that there is a space between the keyword and the opening parenthesis is relaxed, in practice, for this particular usage. So, the textual overhead per translatable string is reduced to only three characters: the underline and the two parentheses. However, even if GNU gettext uses this convention internally, it does not offer it officially. The real, genuine keyword is truly gettext indeed. It is fairly easy for those wanting to use _ instead of gettext to declare […] #define _(String) gettext (String) instead […].

So you can see the pattern here. _ is the historical idiom, which seems to be increasingly falling out of favor and being replaced by gettext with the older _ idiom being retained as an alias for compatibility. And this practice is predominantly a C practice which has clearly spread to other languages and runtimes.

So given the above, I thought it best to continue in the spirit of Kohsuke's design by evolving it in concert with how gettext usage has been evolving in recent years.

[jglick]

I also assumed you were following the GNU library convention (and I believe that was KK’s intent). getText would be more idiomatic in Java (or Groovy) but then again get_text would be more idiomatic in C!

As to whether we ought to advocate switching from _ to gettext in Groovy views, I have no strong opinion though I tend to leave well enough alone. If Groovy ever deprecates/blocks _ as an identifier and we actually update Jenkins core to use that version, then this would be important. I generally discourage use of Groovy views anyway—the output is (almost always) HTML, so the embedded DSL of Jelly seems more natural; and it is a bad idea to put any sort of complex scripting (anything beyond accessing properties, if-then-else blocks, looping over collections, and introducing variables for repeated expressions) into a view when you can move such logic into Java classes. (Also, JENKINS-31203.)

[basil]

I also assumed you were following the GNU library convention (and I believe that was KK’s intent). getText would be more idiomatic in Java (or Groovy) but then again get_text would be more idiomatic in C!

Yeah, the further back you go in C/Unix history the more weird stuff you find. Such as creat(2). Ken Thompson was once asked what he would do differently if he were redesigning the UNIX system. His reply: "I'd spell creat with an e."

[timja]

I generally discourage use of Groovy views anyway

FWIW I raised an issue here jenkinsci/design-library-plugin#15 to add some docs discouraging using them