8334166: Enable binary check

[zhaosongzs]

@kevinrushforth said in SKARA-2289, 'In general, our repositories contain source code and not binary files. There are exceptions to this for images and other similar resources, but otherwise the policy for most repos is to avoid binary files'. Skara is able to identify binary files when executing jcheck, but this check is not enabled.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8334166: Enable binary check (Bug - P4)

Reviewers

• Kevin Rushforth (@kevinrushforth - Author)
• Magnus Ihse Bursie (@magicus - Reviewer)
• Phil Race (@prrace - Reviewer)
• Erik Joelsson (@erikj79 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19683/head:pull/19683
$ git checkout pull/19683

Update a local copy of the PR:
$ git checkout pull/19683
$ git pull https://git.openjdk.org/jdk.git pull/19683/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19683

View PR using the GUI difftool:
$ git pr show -t 19683

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19683.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back zsong! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@zhaosongzs This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8334166: Enable binary check

Reviewed-by: kcr, ihse, prr, erikj

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 80 new commits pushed to the master branch:

• 1b1dba8: 8333358: java/io/IO/IO.java test fails intermittently
• a81e1bf: 8332252: Clean up vmTestbase/vm/share
• 99e4d77: 8333117: Remove support of remote and manual debuggee launchers
• 9ef86da: 8334170: bug6492108.java test failed with exception Image comparison failed at (0, 0) for image 4
• 001d686: 8332587: RISC-V: secondary_super_cache does not scale well
• 5cad0b4: 8322708: Global HTML attributes are not allowed
• 6420846: 8334396: RISC-V: verify perf of ReverseBytesI/L
• c6f3bf4: 8334026: Provide a diagnostic PrintMemoryMapAtExit switch on Linux
• cabd104: 8334164: The fix for JDK-8322811 should use _filename.is_set() rather than strcmp()
• d7dad50: 8334544: C2: wrong control assigned in PhaseIdealLoop::clone_assertion_predicate_for_unswitched_loops()
• ... and 70 more: https://git.openjdk.org/jdk/compare/74468bc1f3aff7f53b91e342711dc095d97fdfed...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@magicus, @prrace, @erikj79) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@zhaosongzs The following label will be automatically applied to this pull request:

• build

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (75ae81f6)

[kevinrushforth]

How is this check presented to the PR author and Reviewers of a PR? Is it listed as a warning similar to the issue title warning? Do you have a sample PR (Draft is fine) that would show how this looks?

[zhaosongzs]

How is this check presented to the PR author and Reviewers of a PR? Is it listed as a warning similar to the issue title warning? Do you have a sample PR (Draft is fine) that would show how this looks?

openjdk/playground#208
Here is a sample PR. Maybe we need to rephrase the warning message.

[prrace]

How is this check presented to the PR author and Reviewers of a PR? Is it listed as a warning similar to the issue title warning? Do you have a sample PR (Draft is fine) that would show how this looks?

openjdk/playground#208 Here is a sample PR. Maybe we need to rephrase the warning message.

I agree, the phrasing makes it sound like a hard error, not a warning, how about

"Binary files checked into project source repositories are strongly discouraged, you should consider alternative solutions."

But why does your example warn on a .png ? They are supposed to be allowed ..

[zhaosongzs]

How is this check presented to the PR author and Reviewers of a PR? Is it listed as a warning similar to the issue title warning? Do you have a sample PR (Draft is fine) that would show how this looks?

openjdk/playground#208 Here is a sample PR. Maybe we need to rephrase the warning message.

I agree, the phrasing makes it sound like a hard error, not a warning, how about

"Binary files checked into project source repositories are strongly discouraged, you should consider alternative solutions."

But why does your example warn on a .png ? They are supposed to be allowed ..

The BinaryCheck in SKARA does not differentiate between types of binary files, it warns for all binary files.
But it's just a warning, so if the reviewers think it's ok, you can integrate your pr with the warning.

[kevinrushforth]

What I think we want here is an informational message for Reviewers rather than a warning. So a message in the Description that says something like "Note: this PR includes binary files" might be sufficient. I don't see the need to be more nuanced than that by checking the type of binary files.

[prrace]

If skara really is unable to distinguish a png from an exe, then I agree the warning needs to be toned way down to an "FYI", not even a warning.
And something less scary than the yellow triangle with a "!" as the graphic too.

[zhaosongzs]

If skara really is unable to distinguish a png from an exe, then I agree the warning needs to be toned way down to an "FYI", not even a warning. And something less scary than the yellow triangle with a "!" as the graphic too.

I don't think we should remove the warning icon(yellow triangle with a "!"), if so, I bet someone will complain that the warning is not eye-catching enough...

[magicus]

@prrace We don't add png files that often, and when we do, I think we can live with a warning sign.

But I agree that the text needs to be more clear that this is not necessarily forbidden, but requires an additionl check and confirmation that it is okay in this case.

[zhaosongzs]

The related Skara enhancement has been integrated and now is live.
openjdk/playground#208

[magicus]

This looks much better. I think we can enable this check for the JDK repo, but let's hear what the others say as well.

[magicus]

/reviewers 2

[openjdk]

@magicus
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[kevinrushforth]

This looks great now.

[kevinrushforth]

@prrace Any objections to the current version of the warning message?

[prrace]

@prrace Any objections to the current version of the warning message?

Meaning this example " ⚠️ Patch contains a binary file (duke-thinking.png)"

I think that's fine.

[kevinrushforth]

@prrace Any objections to the current version of the warning message?

Meaning this example " ⚠️ Patch contains a binary file (duke-thinking.png)"

Yeah, that one.

[zhaosongzs]

Thank you all for the review!
/integrate

[openjdk]

@zhaosongzs
Your change (at version 75ae81f) is now ready to be sponsored by a Committer.

[erikj79]

/sponsor

[openjdk]

Going to push as commit a4582a8.
Since your change was applied there have been 105 commits pushed to the master branch:

• 7baddc2: 8334339: Test java/nio/file/attribute/BasicFileAttributeView/CreationTime.java fails on alinux3
• eb110bd: 8334180: NMT gtests introduced with 8312132 should be labeled as NMT
• 652784c: 8334392: Switch RNG in NMT's treap
• 72ca7ba: 8334708: FFM: two javadoc problems
• 7e55ed3: 8333748: javap crash - Fatal error: Unmatched bit position 0x2 for location CLASS
• 1ff5acd: 8332099: since-checker - Add @ since to package-info in jdk.jsobject
• 689cee3: 8334509: Cancelling PageDialog does not return the same PageFormat object
• 8e1d2b0: 8334441: Mark tests in jdk_security_infra group as manual
• 93d9802: 8334715: [riscv] Mixed use of tab and whitespace in riscv.ad
• c41293a: 8334695: Fix build failure without zgc after JDK-8333300
• ... and 95 more: https://git.openjdk.org/jdk/compare/74468bc1f3aff7f53b91e342711dc095d97fdfed...master

Your commit was automatically rebased without conflicts.

[openjdk]

@erikj79 @zhaosongzs Pushed as commit a4582a8.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[robm-openjdk]

/backport jdk23u

[openjdk]

@robm-openjdk the backport was successfully created on the branch backport-robm-openjdk-a4582a89-master in my personal fork of openjdk/jdk23u. To create a pull request with this backport targeting openjdk/jdk23u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit a4582a89 from the openjdk/jdk repository.

The commit being backported was authored by Zhao Song on 24 Jun 2024 and was reviewed by Kevin Rushforth, Magnus Ihse Bursie, Phil Race and Erik Joelsson.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk23u:

$ git fetch https://github.com/openjdk-bots/jdk23u.git backport-robm-openjdk-a4582a89-master:backport-robm-openjdk-a4582a89-master
$ git checkout backport-robm-openjdk-a4582a89-master
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk23u.git backport-robm-openjdk-a4582a89-master

⚠️ @robm-openjdk You are not yet a collaborator in my fork openjdk-bots/jdk23u. An invite will be sent out and you need to accept it before you can proceed.