conan-io / docs

conan.io reStructuredText documentation
http://docs.conan.io
MIT License
106 stars 356 forks source link

[feature] Remove or document effect of EOL on recipe revisions #1518

Open kyllingstad opened 4 years ago

kyllingstad commented 4 years ago

When a recipe revision is calculated as a hash of exported files (revision_mode = hash), different EOL conventions will lead to different recipe hashes. This can cause problems that are difficult to figure out, as I describe below. I suggest that you do one of the following:

The case that motivated this report, in case you're interested:

We recently had an issue where Conan kept insisting on building a package from source rather than download a prebuilt binary package from Artifactory. We verified that the package IDs were equal, so we knew that the build settings and options were the same. After quite a bit of investigation, we figured out that the packages on Artifactory had been uploaded with different recipe revisions for the Windows and Linux builds. In our case, the Windows package had been built slightly later than the Linux package, and therefore became the "most recent" one, so when we then tried to use the package on Linux, Conan couldn't find binaries for the most recent RREV. As it turned out, the whole thing was caused by a file that had been added to the exports, which got checked out with CRLF endings on the Windows build nodes and LF endings on the Linux build nodes.

memsharded commented 4 years ago

Hi @kyllingstad

The truth is that we have thought about this a few times, and always rejected the idea, because the recipe is not only composed by the conanfile.py, but also other files: the manifest, but also other files that are exported (exports or exports_sources). All the exported files, it is really dangerous to mess with EOL. You never know what type of files, you can deduce some by the extension, but even in that case, it is impossible to know what is the user intent. I am quite confident that many users don't want Conan to mess with the EOL of their source files.

Then, we decided to delegate this on the user and the source control. Git has already configuration for managing this, if the files that gets packaged into Conan packages are different is really because they were checked-out differently by source control. With the support of basically all modern IDEs and tools, it seems it is becoming less necessary to checkout with different line endings.

kyllingstad commented 4 years ago

I understand and agree, and I think that a prominent warning in the package revisions documentation would be a satisfactory solution.

memsharded commented 4 years ago

Good! Moved this to the documentation, lets add there a visible warning.

Nocccer commented 3 years ago

I had the same issue and thought i can avoid it if i use revision_mode="scm". It will work as long as you don't call "conan remove --outdated". Everytime the line ending of the recipe change, conan will upload the "new" recipe and marks all packages related to the "older" recipe as outdated.

Maybe the package recipe is not updated, if only the line ending change? Is there already an option for it ?

chausner commented 3 years ago

Although we have run into this issue multiple times already and have internal documentation that requires us to use a .gitattributes file to normalize the line endings, sometimes you simply forget to add it in some repo and then the behaviour is very confusing and hard to debug until you remember what the issue was.

Things like these really increase the learning curve of Conan IMO and prevent new people from efficiently getting work done. And even for myself who has been working with Conan almost daily for 9 months now it can still be confusing because you easily forget the issue and then after some time run into it again.

So overall I would appreciate if Conan could handle this in some better way.

memsharded commented 3 years ago

So overall I would appreciate if Conan could handle this in some better way.

@chausner the thing is that I have already implemented this thing. It was a huge pain to maintain, and it produced many issues to users. In the surface it seems doable, but once you start applying internal logic, trying to deduce what is a text file and what not, and then different LF that are indeed very relevant for many build systems and other programs, then suddenly you are either breaking them, or not allowing to do fixes to them because if internally normalizing, the computed revision will always be the same, and that fix you did in your Makefile regarding CRLF will not be uploaded anymore. I can guarantee almost as much time debugging and suffering from this approach than with the current one. It is really not that I oppose to do it, it is that the solution is not that evident to actually help users.

And any solution that we would implement most likely will be extremely similar to what git is already doing. It will be necessary a file to identify which files are text and which not. Then users will complain that they forgot to annotate in the file some new text file and that this is broken, and why not just reading the .gitattributes file instead so they only need to annotate it in one place... I mean, if the problem is the different LF, and git already has a mechanism that handle exactly this problem, why not leveraging it? How would it be different to have a custom (I can guarantee way inferior) implementation of it in Conan?

solvingj commented 3 years ago

@kyllingstad I did not see this ticket until now, but I came to the same conclusion and added this:

https://github.com/conan-io/docs/pull/2028

I'm going to guess that the reason nobody else on our team added this to the docs before now is that we've had this document for a long time, which basically explains the problem with line endings.

https://docs.conan.io/en/latest/faq/using.html#packages-got-outdated-when-uploading-an-unchanged-recipe-from-a-different-machine

However, this was written before revisions existed, and back then the error produced was that the package was outdated . So, most people, when reading about, implementing, and searching for errors about revisions don't generally see this document, and the relationship is not obvious.

chausner commented 3 years ago

@memsharded Thanks for your quick response! Too bad to hear that there does not seem to be any easy solution. Still, my hope is that maybe at least diagnostics can be improved in the future so that it is easier to detect when someone runs into this issue. Having a warning about this in the docs certainly helps but it is not enough in my opinion. I have no idea what is technically feasible, so if you say there is no solution at all, I believe you of course.

memsharded commented 3 years ago

I agree, maybe having a warning is very doable and could help users enough. Wdyt @solvingj?

solvingj commented 3 years ago

Yes I was thinking about it, but no obvious place/time/mechanism for such a warning comes to mind. The problem is that, two (more than 2 usually) different build machines export the same commit at roughly the same time, but get two different Conan revisions. The build machines are typically intentionally independent from each other, so how do you detect any conflicting state between them? Such a detection would have to involve Conan server, but that immediately suggests some dedicated new step to be engineered on the user side. I don't see it in there.

Getting really creative, I could see a new "global policy" in conan.conf which produces a warning when exporting if any of the files have @CRLF (or @LF). But, all it could say is:

"Hey, I see you're using windows line feeds and revisions_enabled=1, you should be aware of the following situation that can occur under these circumstances..."

It's SUPER awkward.

To be honest, using revision_mode=scm sets the recipe revision to precisely match the SVN revision. This means that, regardless of the line endings, the recipe revision is the same. I haven't explored this mode to see if there are any disadvantages... to be honest I don't like it very much in principle. But, I guess if it works, it is one way to avoid the problem which we could document.

Still, I have no silver bullet idea to help people detect that this is the root cause of their issue when they see the symptoms.