renovatebot / renovate

Home of the Renovate CLI: Cross-platform Dependency Automation by Mend.io
https://mend.io/renovate
GNU Affero General Public License v3.0
17.6k stars 2.32k forks source link

Maven Updates Not Resolving Correct Position when running cross platform (windows with git bash) #6745

Closed hazendaz closed 2 years ago

hazendaz commented 4 years ago

What Renovate type are you using?

Self-hosted Renovate. Using the latest 21.29.1

Describe the bug

Typically I use attributes which are allowed on properties within maven such as.

3.6.1 **Relevant debug logs** The raised pull requests that work are not logging in right location which wipes out tags and sometimes is lines off from where it should be. The logs are not super consistent as the line it uses doesn't always end up the one it reports on but what is seen is that the "version" denoted is not a version at all. I see stuff like "A partial attribute comment in the tag followed by a version number and at other times no version number at all, just comments". This results in project failing to raise many of the pull requests but ones that do are incorrectly setting wrong lines, breaking tags, etc. As a reference, dependabot does not have this such issue. **To Reproduce** I do not yet have a sample on github, but you could take https://github.com/hazendaz/base-parent/blob/master/pom.xml which is mine and try to run the update. This uses attributes for notes on 2 versions and one of those does have an update that is availalble and I suspect will trip this up. **Additional context** I believe but have no evidence that the attributes are tripping up positioning of the version to apply resulting in this issue. I have looked around at prior issues and haven't found anything that would indicate why this condition occurs. I however suspect not many use attributes this way and instead use comments. The reason for using allowable attributes is to allow auto formatting of poms that do not move comments that otherwise bloat the files. Anyway, I'm clueless otherwise why this is not working as expected.
viceice commented 4 years ago

Had a short look at maven manager. It uses it's own update function instead of the auto replace strategy.

We should refactor it to use auto replace, which should be easily doable.

@hazendaz Please create a minimal reproduction repo to test / debug against.

rarkins commented 4 years ago

@viceice there's one thing holding back autoReplace - updates may need to be done in another file, not packageFile. I plan to support this by letting a manager extract function return an optional field called editFile which tells us which file to replace in if not the packageFile.

Also then need to think how to achieve autoReplace with extractAllPackageFiles()

hazendaz commented 4 years ago

@viceice Is there any difference in how this is handled from a perspective of running self-hosted vs using github integration? The reason I ask is that I set up an example using github integration and didn't see the problem. It could be that I just have not exposed it in the example I have. If these are to act the same, I'll dig more. If not, I'll get self-hosted working against github instead. Thanks in advance.

hazendaz commented 4 years ago

Here is the one I ran on github integration that worked fine for reference.

https://github.com/hazendaz/dependency-tracker/pull/520/files

rarkins commented 4 years ago

It should be the same

viceice commented 4 years ago

@hazendaz Can you post the DEBUG logs for the failed cli run?

hazendaz commented 4 years ago

@viceice @rarkins I re-titled this issue to more reflectively note what it is.

I got around this by running on a jenkins server on linux.

I'm unable to share the setup but its root cause seems pretty easy to understand and maybe be fixed.

Steps to reproduce

So my gut tells me that it's getting confused because I'm on windows, have a git attributes set to auto that should have carriage line feeds but they are using *nix line feeds. That count happens to add up to exactly how far down it decides to apply the offset version.

I've worked around the issue at this point and I'm up and running when entirely linux. Hopefully above is enough to go on that might help flush out the issue. The only thing this doesn't allow me to do currently is run this locally in this situation if I need so which is unlikely now for me personally. This may be useful for others though as my initial need was to setup it locally before messing with jenkins and trying to run on the cloud.

beatngu13 commented 2 years ago

We can confirm the problem with the current version of Renovate (self-hosted on Windows). However, the workaround described here by @ngeor works like a charm:

$ git config --global core.autocrlf input
PhilipAbed commented 2 years ago

that sounds like the autoclrf windows problem with carriage return \r\n, please see: https://github.com/renovatebot/renovate/blob/main/docs/development/best-practices.md#windows

PhilipAbed commented 2 years ago

@rarkins should we turn this into a discussion? or do you still wanna do a refactor of somekind?

rarkins commented 2 years ago

If we have a way of fixing this automatically (such as setting autocrlf before cloning) then let's do it, otherwise we should just document it as a requirement for running Renovate on Windows (whether in dev or production)

PhilipAbed commented 2 years ago

the only way to resolve this is removing carriage returns manually right after cloning ,

i tried to look for a way or another library to git clone / checkout with atuocrlf but i can't find any, someone even asked a question in stack overflow for this https://stackoverflow.com/questions/57916906/npm-package-lf-or-crlf-or-auto-line-endings

rarkins commented 2 years ago

If we can't do it automatically, let's not do it and instead document it.

PhilipAbed commented 2 years ago

we have it documented in https://github.com/renovatebot/renovate/blob/main/docs/development/best-practices.md#windows but we need it somewhere where everybody could read it, @HonkingGoose can you help here? where do you think we should add this knowledge so that everybody can see it? we have had over 5 tasks open on bad replacement location already and we always close them with this work around. lets avoid extra issues. I thought about opening a discussion and answering it so that people can see, but i'm not sure they know what to search for as bad replacement location -> Windows auto crlf aren't aligned, we need like "Known Limitations" in documentation

HonkingGoose commented 2 years ago

@HonkingGoose can you help here? where do you think we should add this knowledge so that everybody can see it?

I'll go think about that a bit now.

we have had over 5 tasks open on bad replacement location already and we always close them with this work around. lets avoid extra issues.

If people keep opening issues, it means we're not documenting things properly. 😄

I thought about opening a discussion and answering it so that people can see, but i'm not sure they know what to search for as bad replacement location -> Windows auto crlf aren't aligned, we need like "Known Limitations" in documentation

Having a discussion with the answer in it would be nice to have as well. But the primary source should always be our documentation. Discussions can go stale/outdated, and people won't know that the answer might be wrong now.

We have a Known Limitations in our docs already.

HonkingGoose commented 2 years ago

Questions

  1. Who are affected by the "Windows auto crlf" problem, people who self-host Renovate, people who use our hosted app, or both?
  2. How important is it that people know about this problem? I'm trying to figure out if we should put it near the top of a page, or later down the page.
  3. What's our recommendation on how to set up the core.autocrlf feature of Git?

Copy/paste relevant section of the ProGit2 book: [^progit2]

Formatting and Whitespace

Formatting and whitespace issues are some of the more frustrating and subtle problems that many developers encounter when collaborating, especially cross-platform. It's very easy for patches or other collaborated work to introduce subtle whitespace changes because editors silently introduce them, and if your files ever touch a Windows system, their line endings might be replaced. Git has a few configuration options to help with these issues.

core.autocrlf

If you're programming on Windows and working with people who are not (or vice-versa), you'll probably run into line-ending issues at some point. This is because Windows uses both a carriage-return character and a linefeed character for newlines in its files, whereas macOS and Linux systems use only the linefeed character. This is a subtle but incredibly annoying fact of cross-platform work; many editors on Windows silently replace existing LF-style line endings with CRLF, or insert both line-ending characters when the user hits the enter key.

Git can handle this by auto-converting CRLF line endings into LF when you add a file to the index, and vice versa when it checks out code onto your filesystem. You can turn on this functionality with the core.autocrlf setting. If you're on a Windows machine, set it to true -- this converts LF endings into CRLF when you check out code:

$ git config --global core.autocrlf true

If you're on a Linux or macOS system that uses LF line endings, then you don't want Git to automatically convert them when you check out files; however, if a file with CRLF endings accidentally gets introduced, then you may want Git to fix it. You can tell Git to convert CRLF to LF on commit but not the other way around by setting core.autocrlf to input:

$ git config --global core.autocrlf input

This setup should leave you with CRLF endings in Windows checkouts, but LF endings on macOS and Linux systems and in the repository.

If you're a Windows programmer doing a Windows-only project, then you can turn off this functionality, recording the carriage returns in the repository by setting the config value to false:

$ git config --global core.autocrlf false

[^progit2]: git-scm.com, ProGit 2 book, Customizing Git Configuration

rarkins commented 2 years ago

I think it sounds like a potential problem for anyone who self hosts with windows. We should therefore have it as part of our getting started or installation docs under a Windows subsection

PhilipAbed commented 2 years ago

it only affects self hosted windows, documentation should do, maybe a FAQ for "bad replacement position of version" or something alike, do we have a FAQ?

i think we can move this issue to a discussion or close it

rarkins commented 2 years ago

I think it just should be a prerequisite of running on windows and we don't need to worry people with low level details of failures which won't happen as long as they follow the prerequisites

renovate-release commented 2 years ago

:tada: This issue has been resolved in version 32.101.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: