Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ratchetFrom 'origin/main' does not fetch if not present #710

Open
JLLeitschuh opened this issue Oct 2, 2020 · 28 comments
Open

ratchetFrom 'origin/main' does not fetch if not present #710

JLLeitschuh opened this issue Oct 2, 2020 · 28 comments

Comments

@JLLeitschuh
Copy link
Member

JLLeitschuh commented Oct 2, 2020

If you specify rachetFrom and the branch doesn't exist locally, it should be fetched by the ratchet infrastructure.

https://github.com/JLLeitschuh/spotless/runs/1200916598?check_suite_focus=true#step:5:56

  [2020-10-02 21:07:06] [autobuild] * What went wrong:
  [2020-10-02 21:07:06] [autobuild] A problem occurred evaluating project ':ide'.
  [2020-10-02 21:07:06] [autobuild] > Could not create task ':lib:spotlessJava'.
  [2020-10-02 21:07:06] [autobuild]    > No such reference 'origin/main'

EDIT: Summary of workarounds from below

  • GitHub Actions: add fetch-depth: 0 to <action>.yml
  • GitLab CI: add GIT_DEPTH: 0 under the variables: section of .gitlab-ci.yml
  • BitBucket Pipelines: clone: depth: full
  • Travis: git: depth: false
@nedtwigg
Copy link
Member

nedtwigg commented Oct 3, 2020

Interesting, good idea! I think GitHub is doing clone --depth=1. Doing a fetch origin main right after sort of defeats the purpose of the shallow clone, and the user is probably better off performance-wise doing something like fetch-depth: 0 (code for deep clone) , which does a full clone in the first place.

Even so, I still think it's better to print a warning like "This is silly, you should just do a full clone to start. No worries though, we'll do the fetch ourselves" and just fix it ourselves, as you suggest.

The clever thing would be to do fetch --depth=5 for both origin main and HEAD. 5 might not be enough though, we're actually formatting based on the merge head. But you could do an doubling thing 5,10,20,40,80,etc. until you find the merge head. It's probably not worth the complexity. Happy to take a PR for either approach. The code in question is this:

ObjectId commitSha = repo.resolve(reference);
if (commitSha == null) {
throw new IllegalArgumentException("No such reference '" + reference + "'");
}
RevCommit ratchetFrom = revWalk.parseCommit(commitSha);
RevCommit head = revWalk.parseCommit(repo.resolve(Constants.HEAD));
revWalk.setRevFilter(RevFilter.MERGE_BASE);
revWalk.markStart(ratchetFrom);
revWalk.markStart(head);
RevCommit mergeBase = revWalk.next();

@JLLeitschuh
Copy link
Member Author

I spent ~ 10 min poking at the code and the JGit API. I really don't feel like I have the depth of knowledge here of how Git works to appropriately fix this issue.

The reason that I opened this PR was mostly to try to support this pull request #711. I actually don't need this for my own uses of spotless.

@nedtwigg
Copy link
Member

nedtwigg commented Oct 5, 2020

Fair enough :). Can you fix #711 with fetch-depth: 0 (note that the link is to github actions documentation).

@JLLeitschuh
Copy link
Member Author

Fair enough :). Can you fix #711 with fetch-depth: 0 (note that the link is to github actions documentation).

Done! Would you like to keep this issue open in case some enterprising individual comes along and wants to resolve this, or should we close it out?

@nedtwigg
Copy link
Member

nedtwigg commented Oct 5, 2020

Let's keep it open, I still think it's a good idea.

@prudhvir3ddy
Copy link

😅 Guys, facing same and i have other issues to fix.

TBH ratchet from got no use if it doesn't work on CI ?

bcoz usually we check lints at ci.

or do we have something like baseline file options where that file can store current lint errors and only errors that's not their in the file is reported ?

@nedtwigg
Copy link
Member

nedtwigg commented Nov 9, 2020

It works in CI, but it doesn't work on a bare or --depth=1 checkout. Most CI systems (for sure GitHub and GitLab) allow for full checkout, so that's the workaround. Implementing this is only a performance improvement, and only for some CI systems.

@Xendar
Copy link

Xendar commented Nov 12, 2020

I had the same issue with my CI (Jenkins) using Bitbucket for Git.
For multibranch pipelines Jenkins will fetch only the branch associated to the current build. So I added a manual fetch of my main branch in my pipeline to be sure it's there

@JLLeitschuh
Copy link
Member Author

This sounds like an optimization that is implemented by-default into a majority of CI tools.

@nedtwigg
Copy link
Member

is implemented by-default into a majority of CI tools.

I agree, but I still think it would still be nice if Spotless fixed it automatically. If the branch that Spotless needs isn't there, it can fetch it as well as anybody.

@driv
Copy link
Contributor

driv commented Nov 13, 2020

I might be able to take a look.

What is the idea here then?

  • The CI tool has to do a proper fetch with history. This is not going to be spotless responsibility.
  • If the base branch is not present, a fetch is tried.

@nedtwigg
Copy link
Member

Spotless is looking for the merge base. So it needs the history of ratchetFrom, and it also needs the history of HEAD, at least deep enough to find the intersection of those two things.

The idea is that if any history is missing, whether of HEAD or of ratchetFrom, it would be better for Spotless to just fix it with a fetch rather than fail, regardless of whether it is HEAD or ratchetFrom (or both) which is missing.

The place where code will need to be changed is described in #710 (comment)

@driv
Copy link
Contributor

driv commented Nov 19, 2020

After some investigation it seems to me that shallow clones don't yet play nicely with jgit.

From what I understand you can't just fetch from a shallow clone, you have to either: git fetch [remote branch] --unshallow or git fetch [remote branch] --depth n.

A different story is if just the current branch has been fetched, it should not be a problem to retrieve the base branch (main). I'll focus on this for now.

@nedtwigg
Copy link
Member

Using command-line git is an option too. We use it here

oldYear = parseYear("git log --follow --find-renames=40% --diff-filter=A", file);

If you go this route, I would use ProcessRunner to handle the shell output. Someday we should refactor LicenseHeaderStep to use this too.

brevilo added a commit to brevilo/jolm that referenced this issue Sep 25, 2021
* Workaround until spotless fetches ratchetFrom base automatically
* Details: diffplug/spotless#710
garrettsummerfi3ld added a commit to GalaxeTV/GalaxeSMP that referenced this issue May 20, 2022
CI would fail due to spotless not doing a full checkout.

More information here: diffplug/spotless#710

Fix clickEvent not being reachable.

Added copyright holder.
@EricGao888
Copy link
Contributor

EricGao888 commented Aug 10, 2022

Another question about the workaround is usually we use fork-pull model during open-source contributions, which means we have upstream/main in local and origin/main in remote. If I don't want to set ratchetFrom as HEAD, I set it as upstream/main locally. After I push it to the remote, CI fails because the remote one should be origin/main instead of upstream/main. May I ask whether there is also a workaround for this? Thanks! @nedtwigg

https://github.com/sevntu-checkstyle/sevntu.checkstyle/wiki/Development-workflow-with-Git:-Fork,-Branching,-Commits,-and-Pull-Request

@nedtwigg
Copy link
Member

I would use environment variables and do something like:

String ratchetAnchor = if (System.getenv('CI') == 'true') 'origin/main' else 'upstream/main'
spotless {
  ratchetFrom ratchetAnchor

@EricGao888
Copy link
Contributor

I would use environment variables and do something like:

String ratchetAnchor = if (System.getenv('CI') == 'true') 'origin/main' else 'upstream/main'
spotless {
  ratchetFrom ratchetAnchor

@nedtwigg Pretty cool, thanks!

@nedtwigg
Copy link
Member

JGit 6.3 added support for manipulating shallow clones. We would need to bump our minimum JRE to 11 to use JGit 6.3, but I'd be happy to do that if someone needs it to "fix" shallow clones to include the refs we need.

@ericos-bennett
Copy link

In case anyone runs into this issue with BitBucket Pipelines, adding clone: depth: full to the build step resolved it for me.

@simararneja
Copy link

Anyone using travis can override by adding the following in the travis.yml

git:
  depth: false

@jratt0
Copy link

jratt0 commented Oct 3, 2023

Easy short-term fix?

→ Internally convert all spotless tasks to no-ops when the ratchetFrom branch can't be found.

  1. Avoids
    1. Pulling extra data during CI/CD
    2. Manually working around alternate branch names
  • Even on full clones, our CI/CD system doesn't appear to create an origin/upstream remote ref.
  • We aren't overly concerned with checking formatting in the CI/CD pipeline. We are happy having integrated spotlessApply into the local build steps.

@nedtwigg
Copy link
Member

nedtwigg commented Oct 3, 2023

I think you could accomplish the above with

spotless {
  enforceCheck env['CI'] != 'true'

@jratt0
Copy link

jratt0 commented Oct 3, 2023

(I'm probably going off-topic for this issue, but appreciate the quick response!)

I think you could accomplish the above with

spotless {
  enforceCheck env['CI'] != 'true'

I don't think that works for us: we are running spotlessApply during the build and have already disabled spotlessCheck using enforceCheck. OTOH, we could use the check-env approach in our custom rules that added spotlessApply into the basic build

@helloncode
Copy link

helloncode commented Nov 9, 2023

Currently, Spotless (Gradle Plugin) performs a check for the presence of the ratchetFrom branch on every command execution. This includes instances where Spotless-specific commands aren't called, which can lead to unnecessary checks, and adds the workaround for all cases.

Could we refine the ratchetFrom functionality so that it checks for branch presence only when a Spotless command is actually invoked?

I believe this adjustment could be beneficial for users who integrate Spotless into their development workflows, ensuring that the tool remains as efficient and unobtrusive as possible.

Let me know if your prefer a new issue on this topic

@nedtwigg
Copy link
Member

@helloncode happy to take a PR for this, a new issue is probably better. I will delete my comment here and your comment above at some point to keep this issue more focused.

@helloncode
Copy link

helloncode commented Nov 23, 2023

@helloncode happy to take a PR for this, a new issue is probably better. I will delete my comment here and your comment above at some point to keep this issue more focused.

@nedtwigg Opened a new issue #1902.
Thank you

abendt added a commit to abendt/aws-kinesis that referenced this issue Dec 1, 2023
@superxiao
Copy link

On the maven side, if you skip spotless (-Dspotless.skip=true), it won't require the default branch to be fetched. Is it possible to do something similar here?

@omhack
Copy link

omhack commented Sep 12, 2024

If you specify rachetFrom and the branch doesn't exist locally, it should be fetched by the ratchet infrastructure.

https://github.com/JLLeitschuh/spotless/runs/1200916598?check_suite_focus=true#step:5:56

  [2020-10-02 21:07:06] [autobuild] * What went wrong:
  [2020-10-02 21:07:06] [autobuild] A problem occurred evaluating project ':ide'.
  [2020-10-02 21:07:06] [autobuild] > Could not create task ':lib:spotlessJava'.
  [2020-10-02 21:07:06] [autobuild]    > No such reference 'origin/main'

EDIT: Summary of workarounds from below

  • GitHub Actions: add fetch-depth: 0 to <action>.yml
  • GitLab CI: add GIT_DEPTH: 0 under the variables: section of .gitlab-ci.yml
  • BitBucket Pipelines: clone: depth: full
  • Travis: git: depth: false

what about Azure?
anyone?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests