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

Avoid using project.name for ignored projects check #262

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Goooler
Copy link
Contributor

@Goooler Goooler commented Jul 24, 2024

The project's name is not necessarily unique within a project hierarchy. You should use the getPath() method for a unique identifier for the project.

See https://docs.gradle.org/current/javadoc/org/gradle/api/Project.html#getName().

@Goooler
Copy link
Contributor Author

Goooler commented Jul 24, 2024

2 tests failed, seems they are related to #261.

kotlinx.validation.test.MultiPlatformSingleJvmTargetTest > testApiCheckFails FAILED
    java.lang.AssertionError at MultiPlatformSingleJvmTargetTest.kt:84


* What went wrong:
Execution failed for task ':jvmApiCheck'.
> API check failed for project :.
  --- /private/var/folders/_8/1blqtzds07g8p1zsy_t8_lsc0000gn/T/junit8648894727794548688/api/testproject.api
  +++ /private/var/folders/_8/1blqtzds07g8p1zsy_t8_lsc0000gn/T/junit8648894727794548688/build/api/testproject.api
  @@ -1,9 +1,9 @@
  -public final class com/company/subsub2/Subsub2Class {
  +public final class com/company/subsub1/Subsub1Class {
   	public fun <init> ()V
   	public final fun getProp ()I
   }
   
  -public final class com/company/subsub1/Subsub1Class {
  +public final class com/company/subsub2/Subsub2Class {
   	public fun <init> ()V
   	public final fun getProp ()I
   }
  
  You can run :::apiDump task to overwrite API declarations
kotlinx.validation.test.MultipleJvmTargetsTest > testApiCheckFails FAILED
    java.lang.AssertionError at MultipleJvmTargetsTest.kt:99


* What went wrong:
Execution failed for task ':jvmApiCheck'.
> API check failed for project :.
  --- /private/var/folders/_8/1blqtzds07g8p1zsy_t8_lsc0000gn/T/junit8084192555986675768/api/jvm/testproject.api
  +++ /private/var/folders/_8/1blqtzds07g8p1zsy_t8_lsc0000gn/T/junit8084192555986675768/build/api/jvm/testproject.api
  @@ -1,9 +1,9 @@
  -public final class com/company/subsub2/Subsub2Class {
  +public final class com/company/subsub1/Subsub1Class {
   	public fun <init> ()V
   	public final fun getProp ()I
   }
   
  -public final class com/company/subsub1/Subsub1Class {
  +public final class com/company/subsub2/Subsub2Class {
   	public fun <init> ()V
   	public final fun getProp ()I
   }
  
  You can run :::apiDump task to overwrite API declarations

@Goooler Goooler force-pushed the correct-ignored-project-name branch from f7a243a to 33be62c Compare July 24, 2024 12:16
@fzhinkin fzhinkin self-requested a review August 12, 2024 19:29
@Goooler Goooler force-pushed the correct-ignored-project-name branch from 33be62c to 3789ed7 Compare August 14, 2024 08:49
@Goooler Goooler force-pushed the correct-ignored-project-name branch from 3789ed7 to 4fe69a8 Compare August 14, 2024 08:53
> The project's name is not necessarily unique within a project hierarchy. You should use the getPath() method for a unique identifier for the project.

See https://docs.gradle.org/current/javadoc/org/gradle/api/Project.html#getName().
@Goooler Goooler force-pushed the correct-ignored-project-name branch from 4fe69a8 to b42f190 Compare August 14, 2024 09:18
@fzhinkin
Copy link
Collaborator

@Goooler thanks for taking care of the problem!

While paths are better for identifying projects, there's already plethora of projects using BCV and relying on ignoredProjects property.
We can follow the suggestion from #16 on how to preserve compatibility, and treat each of ignoredProjects values either as a project name, or as a project path depending on the first character (assuming that only the project path could start with :).
Additionally, it would nice to print a warning for every ignoredProjects entry that is no a project path (so that users gradually migrate to project paths and we can drop project names support in the future).

@Goooler
Copy link
Contributor Author

Goooler commented Aug 19, 2024

This might be a breaking change, but it would be worth breaking in the next release, users who want to update should update their ignoredProjects instead of treating it as a warning.

Yeah, we can remove : prefixes for path checks, but there will be more complex cases for us to treat, such as the root project, path as :. I suggest we should retain the original paths for checks.

@fzhinkin
Copy link
Collaborator

This might be a breaking change, but it would be worth breaking in the next release, users who want to update should update their ignoredProjects instead of treating it as a warning.

It's better to provide a smooth and graceful migration path when possible. Those who already use ignoredProjects are fine with how it works. They'll have to switch to paths at some point, but it should not block them from updating a BCV.
In some cases, a breaking change is required to fix a problem that could not be fixed otherwise. But in case with path/name checks, behavior could be updated smoothly, without breaking what already work.

We just need to transform

if (project.path in extension.ignoredProjects) { ... }

into something like

fun Project.isIgnored(extension.ignoredProjects: ApiValidationExtension) = path in extension.ignoredProjects || name in extension.ignoredProjects

if (project.isIgnored(extension)) { ... }

And also update Project.validateExtension to check that ignoredProjects values are either existing names, or paths (printing a warning for names along the way).

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

Successfully merging this pull request may close these issues.

Subprojects that contain the same name will be ignored Allow ignoring project by path
2 participants