- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.3k
Bundle Compose Hot Reload Gradle plugin into the Compose Gradle plugin #5444
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
Conversation
2195620    to
    a800d3f      
    Compare
  
            
          
                gradle-plugins/compose/src/test/test-projects/application/hotReload/build.gradle
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/ComposePlugin.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting for the approach with the 'preferred' dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach seems elegant to me.
Note: I am not reviewing the tests, but only the mechanics of how the 'bundling' works. Using a preferred versioned dependency seems like a good solution.
4be7a02    to
    4fcf98d      
    Compare
  
            
          
                gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/ComposePlugin.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                gradle-plugins/compose/src/test/test-projects/application/hotReload/build.gradle
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                gradle-plugins/compose/src/test/test-projects/application/hotReload/build.gradle
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
          
            Show resolved
            Hide resolved
        
              
          
                ...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
          
            Show resolved
            Hide resolved
        
      | topics to discuss: 
 | 
ce75c3b    to
    88a3982      
    Compare
  
    | 
 "3." & "4." are addressed here. | 
05aada2    to
    f796ba0      
    Compare
  
            
          
                ...ose/src/main/kotlin/org/jetbrains/compose/desktop/application/internal/configureHotReload.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
      34fdad0    to
    0c5274f      
    Compare
  
            
          
                ...ose/src/main/kotlin/org/jetbrains/compose/desktop/application/internal/configureHotReload.kt
          
            Show resolved
            Hide resolved
        
              
          
                ...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
          
            Show resolved
            Hide resolved
        
              
          
                ...ose/src/main/kotlin/org/jetbrains/compose/desktop/application/internal/configureHotReload.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...lugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/HotReloadTest.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | gradle("prepareKotlinIdeaImport").checks { | ||
| gradle("prepareKotlinIdeaImport", "-P$disabaleProperty=false").checks { | ||
| val expected = if (System.getProperty("os.name").lowercase().contains("windows")) { | ||
| // Windows has different line endings in comparison with Unixes, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ ⚠️ ⚠️ BTW! Does the CHR plugin break reproducible builds between Win and Unix machines?
Can you please elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I could imagine is that different OS create different hashes for resources (which would be a bug?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I could imagine is that different OS create different hashes for resources (which would be a bug?)
Windows has different line endings in comparison with Unixes. So vector.xml vector drawables become binary different when are they checked out from Git and thus produce different hashes.
Last time when we discussed it, we came into conclusion that the hashes must be different, because the content is different and fixed the test by providing different "expected" directories for Windows and non-Windows. However "reproducible builds" sounds like a valid contra-argument so I would handle it in the hash calculation. "Cons" of this approach was that we had to add a custom platform-specific handling logic that sounds fragile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handled this in the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! I strongly agree that any text-based resource should produce the same hashCode regardless of the formatting used. The 'hash' here is not an indicator of the actual text hash, but a hash for its 'semantic'. It should not change if something meaningless changed (e.g., if the format of line-endings changed).
However, I wonder how projects would end up with different XML files in their project? Is there some generation happening that produces different XML files (e.g., uses different line-endings on different platforms)?
        
          
                ...ins/compose/src/main/kotlin/org/jetbrains/compose/resources/GenerateResourceAccessorsTask.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
      2f47039    to
    1bd3f05      
    Compare
  
    …to the compose gradle plugin + use version catalog for the dependency
…oduce a property to disable it. Previously introduced API for enabling its generation is removed.
…tentHash generation.
597fdf8    to
    b097888      
    Compare
  
            
          
                ...ins/compose/src/main/kotlin/org/jetbrains/compose/resources/GenerateResourceAccessorsTask.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | return readText().replace("\r\n", "\n").toByteArray().contentHashCode() | ||
| } else { | ||
| // Once a new text resource file is introduced, we have to catch it and handle its line endings. | ||
| check(type == ResourceType.DRAWABLE || type == ResourceType.FONT) { "Cannot calculate content hash for $type resource type!" } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you check font type?
why should binary PNGs throw the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add all type of resources in the test case, please. to check stability of the hashes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we generate accessors only for font and drawable files (strings and raw files are handled separately). Here I am just checking this fact, so if we introduce a new text resource type, we could catch it and recall that we may need to do something with its line endings. It is reflected in the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add all type of resources in the test case, please.
Do you mean add .svg and raster images?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added two more files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree. If we add a new text based resource type with a new extension, it will fail tests because hashes will be different. We don't need resource type here. And we don't need to check type == ResourceType.DRAWABLE as well. for any file based xml/svg resource we must do conversion on win machines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done.
(let's hope that we don't forget to add tests for new text files)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But just to double-check:
Any project would just check in those text files using a common line-ending, right? Will the git checkout then provide different line endings on different platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
Compose Hot Reload Gradle plugin is bundled with Compose Gradle plugin.
Thus Compose Hot Reload is available from Compose Multiplatform projects with the desktop target.
Fixes CMP-8885
Testing
Integration tests added.
This should be tested by QA.
Release Notes
Highlights - Desktop