-
Notifications
You must be signed in to change notification settings - Fork 64
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
Remove case-insensitive dump file names handling logic #231
Comments
@ZacSweers, @joffrey-bion, @qwwdfsad PTAL. You were all involved in the original issue (#76) discussion, so I'd love to hear your thoughts on the topic. |
Definitely in favor of case-insensitivity 👍 |
@ZacSweers do you have a project that relies on it? |
I don't have any projects that desire case sensitivity. The last issue was that it unexpectedly failed on CI due to CI running linux |
Looking at #76 once again. The problem is how git works on case-insensitive file systems:
|
Wouldn't it "just work(TM)" to make the BCV delete the old file in a case-insensitive manner, and recreate it with an exact match of the project name every time? EDIT: ah sorry that's the case where git would just update the wrongly-cased file in the index instead of renaming it. |
BCV was always deleting the old file, the problem is how git tracks deletion and creation of a new file:
The only proper way to handle file name case change is
|
Yes, understood. Sorry I was too slow to update my comment apparently. I can't believe git works this way by default. This is quite enraging. Now, is it the responsibility of BCV to deal with it? Maybe, maybe not. If we can make the experience better, why not. So, based on what you said, even being strict from the BCV side during I guess that leaves us no choice but to be case insensitive on the |
In general, it's nice to provide a better user experience. However, those who use |
For the record, this is actually not quite correct. Git's default depends on the case-sensitivity of the local OS. So in case-sensitive systems, Reference: https://git-scm.com/docs/git-config#Documentation/git-config.txt-coreignoreCase
Agreed. Although I would argue that if we get annoying Gradle behavior with task invalidation, it's worth reconsidering what experience is best. Basically, if |
Could you please expand on that? If people use |
Well, yeah, that's true. I meant how it works on macOs & Windows.
Not quite. Without
|
I don't quite get that point. I mean, this would happen in all systems, and with all renames, regardless of whether it's just a case change or not. It's just user error with git commands (just doing Also, if the user renames their project (which is the case that leads to this kind of stuff), won't they have to deal with this manually for the directory itself and other files named after the project anyway? |
Assuming that a user uses an IDE to work with a project, if a file rename is performed using GUI, everything will go through underlying VCS's commands, and the rename will be handled correctly. But it's not the case for BCV, where a file is renamed during Gradle's command execution. As a user who renamed a project and rerun
That poses a question: why should BCV handle scenarios when someone forgets to perform some related actions after renaming a project? |
Indeed, and I'm talking about the commit action, not the rename action. For sure,
If this were the case in the IDE, I believe that should be reported as an IDE bug. But my experiment doesn't show this (at least on my Windows machine). Here is my result:
We see both the deleted and the created file, so we're supposed to add both of those and commit both as well. And this works fine. It is even considered a proper rename by git if done in a single commit:
I was mainly talking about the
Exactly. To be frank, BCV is not even supposed to know git exists, let alone the fact that git has any feature related to the filename case. If people don't manage to commit the case change, they should probably open issues with git instead. |
I'm leaning toward API dump filename case mismatch no longer being tolerable on case-sensitive filesystems. The decision could always be reconsidered, but if the issue becomes annoying, I'd rather detach dump file names from project names (#233). |
Filename case mismatch is no longer tolerated on case-sensitive filesystems. Fixes #231
Filename case mismatch is no longer tolerated on case-sensitive filesystems. Fixes Kotlin/binary-compatibility-validator#231 Pull request Kotlin/binary-compatibility-validator#237
Filename case mismatch is no longer tolerated on case-sensitive filesystems. Fixes Kotlin/binary-compatibility-validator#231 Pull request Kotlin/binary-compatibility-validator#237
Filename case mismatch is no longer tolerated on case-sensitive filesystems. Fixes Kotlin/binary-compatibility-validator#231 Pull request Kotlin/binary-compatibility-validator#237
Filename case mismatch is no longer tolerated on case-sensitive filesystems. Fixes Kotlin/binary-compatibility-validator#231 Pull request Kotlin/binary-compatibility-validator#237 Moved from Kotlin/binary-compatibility-validator@4e91d92
Filename case mismatch is no longer tolerated on case-sensitive filesystems. Fixes Kotlin/binary-compatibility-validator#231 Pull request Kotlin/binary-compatibility-validator#237 Moved from Kotlin/binary-compatibility-validator@4e91d92
#79 and #84 addressed the issue reported in #76 by looking up
.api
files in a case-insensitive manner.To make things work smoothly on the Gradle side, we need to:
The latter option should work, but it requires specifying input/output directories and a file name for all the tasks. Otherwise, Gradle won't be able to track dependencies between tasks correctly. Also, all tasks consuming
.api
files from the repo should be updated to support case-insensitive file names (and for Klibs, the majority of tasks need it).Note that before all the klib-related changes, the check task was using directories as its inputs, while the dump task was using files as its output, so the only way to enforce dependency between these tasks was
dependsOn
call.Looking at #76 I'm not sure if macOS's case insensitivity was the root cause. The
:apiDump
task was based onSync
, which wipes out the destination directory before copying files. Even if the dump had a name with the wrong characters case, rerunningapiDump
should have fixed it.So, I propose removing all the logic for case-insensitive file names from the BCV and using exact file paths as Gradle task inputs/outputs.
The text was updated successfully, but these errors were encountered: