search

sghpjuikit / player

Audio player and management application.
22 stars 2 forks source link

Create a separate sourceSet where possible #103

Closed xeruf closed 5 years ago

sghpjuikit commented 5 years ago

One of the recent Gradle updates made it possible to compile separate sourcSets in parallel and I think it makes a lot of sense to make use of that if possible.

I'm thinking at least these sourceSets/modules:

Naturally, the util module will take some work, and I'm going to start moving app-related code out of util.

xeruf commented 5 years ago

Sounds good, yes. I have recently enabled parallel compilation for an Android app that has ~10 little modules, and it was gorgeous to watch my processor churn through all of them in parallel.

xeruf commented 5 years ago

Why did you unassign me though? Isn't this a collaborative thing? We might as well both be assigned? Or do you feel confident enough with Gradle by now?

sghpjuikit commented 5 years ago

Then I look forward to utilizing the parallel compilation. I'm just afraid it wont be possible - from my naive understanding it seems like the modules must be independent from each other to be able to run in parallel.

I need to take care of the app code in util before we can meddle with this. Then Ill assign you. I ask for a little more patience. I started doing good progress on this. About 30% done.

Biggest offender is actually Environment.kt where we will need to use some global state to allow initialization at app start. Not desirable but unavoidable, unless we move Environment.kt from utils, which I do not feel is right at all.

sghpjuikit commented 5 years ago

Did some more work on this.

Leveraging parallel compilation will be possible, source sets demos and app will be independent, both depending on utils. Utils will stay single module/source set.

These are the last util classes that contain application code:

Most of those will require some kind of global 'register' so the app can hook into it at the start. Nothing difficult, but needs to be designed carefully (i.e. singleton y/n?, extensibility?, implement Core?, etc.)

xeruf commented 5 years ago

demos will be independent? That's interesting, but it does make sense.

xeruf commented 5 years ago

Most of those will require some kind of global 'register' so the app can hook into it at the start. Nothing difficult, but needs to be designed carefully (i.e. singleton y/n?, extensibility?, implement Core?, etc.)

Maybe we can do that via Java's SPI. That would also enable classes from the outside to hook into it without directly referencing anything - i.e. independent plugins could easily be attached.

sghpjuikit commented 5 years ago

None of the demos demo application features and all of them serve merely to test/prototype/present certain functionality for developer. They are all self-contained and executable. I do not think there is any need for application demos and if it were, it would be a different module/set - hence it should be independent.

Ok, I will look into SPI, but having worked with it a bit in java.image and java.audio I don's think it will turn out to be a good solution.

xeruf commented 5 years ago

Regardless of whether you use SPI or not, I'd like a solution where stuff can hook in from the outside instead of having to be explicitly mentioned in the Application, because that enables us to separate out stuff as plugins and provides the basics for what an external plugin needs.

sghpjuikit commented 5 years ago

I have been trying to improve already existing sourceSets/modules, but no no avail.

The way I understand this, when using create separate module per source set Intellij maps project to module group and source set to module, but does not support source set having the same content root as the project, which is unfortunately the case with widgets, and this can not be changed due to interfering with package declaration and widget compilation. It's too funny, because in a way we are in this situation due to lack of cross-platform recursive file monitoring support.

I would appreciate little help with these issues.

I have also tried to create a separate source set for each widget and I managed. But it seems Kotlin Gradle DSL lacks the support to define dependencies per source set. This is really disheartening. I could of course define the source sets and dependencies manually, but that would defeat the entire point. My goal was to forbid cross-using widget dependencies.

xeruf commented 5 years ago
  • widgets have test module. This is because "main" and "test" are source sets created by default source. Widgets do not need test source sets! I tried to remove it with sourceSets{ remove(getByName("test") }, which works, but then crashes build with some task complaining the source set is missing. Not sure if bug or feature.

Don't remove a sourceSet. If it does not exist, it will simply be ignored.

  • both player-test and widgets-test modules have wrong default content root, using the src/test idiom. Why cant I override this? Gradle documentation states that calling setSrcDirs() instead of srcDirs += will remove the previous location and override the default idiom, yet I'm im observing the opposite.
sourceSets {
    getByName("main") {
        java.setSrcDirs(listOf("src"))
        resources.setSrcDirs(listOf("src"))
    }
    getByName("test") {
        java.setSrcDirs(listOf("src-test"))
        resources.setSrcDirs(listOf("src-test"))
    }
}

You mean this? I am dying. I have not once seen "test" used as a suffix. It is always src/main and src/test. Please have a look at the structure and gradle buildfile for some of my projects: https://github.com/Xerus2000/monsterutilities https://github.com/Xerus2000/util

  • Because we enable create separate module per source set, Intellij Idea creates 3 actual modules for the project/gradle source set (see here). For widgets this is widgets, widgets-main, widgets-test. For some reason, Intellij sets content root both to widgets and widgets-main, which causes it to complain about duplicate content root and in effect not consider widget source files project files, unless I delete the content root from widgets module in idea project settings or the idea module itself (but then I need to repeat this every time gradle refreshs). It's driving me up the wall.

The way I understand this, when using create separate module per source set Intellij maps project to module group and source set to module, but does not support source set having the same content root as the project, which is unfortunately the case with widgets, and this can not be changed due to interfering with package declaration and widget compilation. It's too funny, because in a way we are in this situation due to lack of cross-platform recursive file monitoring support.

https://github.com/sghpjuikit/player/blob/master/CONTRIBUTING.md#intellij-idea clearly states Disable "Create separate module per source set"

I have also tried to create a separate source set for each widget and I managed. But it seems Kotlin Gradle DSL lacks the support to define dependencies per source set. This is really disheartening. I could of course define the source sets and dependencies manually, but that would defeat the entire point. My goal was to forbid cross-using widget dependencies.

You can define dependencies per sourceSet afaik, but that is a bit tricky. Maybe we could automagically create a module for each widget.

sghpjuikit commented 5 years ago

Don't remove a sourceSet

It was an experiment. Also Gradle documentation states setSrcDirs overrides default source sets, but in my setup that does not work. I'm also getting strange gradle warning which may be related: The DefaultSourceDirectorySet constructor has been deprecated. This is scheduled to be removed in Gradle 6.0. Please use the ObjectFactory service to create instances of SourceDirectorySet instead.

clearly states Disable "Create separate module per source set"

Ah, my bad. For some reason I repeatedly missed the word disable so I always turned it on... Phew. So my problem with main vs test was because of the wrong idea settings.

It is always src/main and src/test

I'm aware, but I did not want to lose git history when I move all source files into a sub-directory. We can still change it to the convention, but...

Maybe we could automagically create a module for each widget.

We can and I already did that and it works, but there is a problem with content root. If each widget will be its own module, its src directory will become the widget directory, but the widget sources use it as a package, which means IDE will complain about wrong package declaration. We could remove the package declaration, but then widgets would 1 compile into wrong directory (can be adjusted in our code) and 2 unable to be loaded with ClassLoader due to ClassNoDefFound due to classes in default package (i.e. no package) being inaccessible (outside of reflection). Well, the warning isn't such a problem, but it does feel hacky now. So the solution would be to set src directory to be the widget's directory's parent, but then multiple modules would have the same content root, which is something Idea complains about when visiting module settings. Again not a problem, but not perfect either.

Note, I achieved this by using Ideas module per source set settings, so I created source sets dynamically. With this option turned off, we would have to make dynamically project per widget. Well, I'm off to try just that. And if Idea complains, well so be it...

sghpjuikit commented 5 years ago

Widgets separated into projects, see a7d8d42. The only problem is mentioned idea package warnings, which is caused by widget projects not able to have the same projectDir, because idea then ignores the files of all but last project.

xeruf commented 5 years ago

It was an experiment. Also Gradle documentation states setSrcDirs overrides default source sets, but in my setup that does not work. I'm also getting strange gradle warning which may be related: The DefaultSourceDirectorySet constructor has been deprecated. This is scheduled to be removed in Gradle 6.0. Please use the ObjectFactory service to create instances of SourceDirectorySet instead.

Have you tried the exact syntax I use in my projects? I never had any such problem.

I'm aware, but I did not want to lose git history when I move all source files into a sub-directory. We can still change it to the convention, but...

You don't lose history. Git is able to detect moves and will associate the history. We have moved stuff around quite a bit in the last few months ^^

We can and I already did that and it works, but there is a problem with content root. If each widget will be its own module, its src directory will become the widget directory, but the widget sources use it as a package, which means IDE will complain about wrong package declaration. We could remove the package declaration, but then widgets would 1 compile into wrong directory (can be adjusted in our code) and 2 unable to be loaded with ClassLoader due to ClassNoDefFound due to classes in default package (i.e. no package) being inaccessible (outside of reflection). Well, the warning isn't such a problem, but it does feel hacky now. So the solution would be to set src directory to be the widget's directory's parent, but then multiple modules would have the same content root, which is something Idea complains about when visiting module settings. Again not a problem, but not perfect either.

Well, the widgets should be entirely independent ideally and able to be written externally, so they shouldn't depend on an arbitrary folder structure resulting from the project.

sghpjuikit commented 5 years ago

The problem was that I let Idea generate modules from source sets. I will take a look at your configurations when I have time. I'm sure it will be a learning experience.

Ah yes, I think I noticed git detected file moving for me in the past. Ok, I will see what I can do, but this has low priority for me right now.

Widgets are independent, we got that covered, it is simply Idea/gradle are a bit inflexible when it comes to multiple projects in the same locations. Reminder: widgets compile into app/widgets directory, not their own directory - that would generate extra directory inside. This is because Linux does not have recursive directory monitoring.

xeruf commented 5 years ago

Reminder: widgets compile into app/widgets directory, not their own directory - that would generate extra directory inside. This is because Linux does not have recursive directory monitoring.

But the class files could be compiled to wherever you want, you don't need to monitor them?

sghpjuikit commented 5 years ago

The monitoring is required for widget hot reloading. It just occurred to me though that the monitoring is already recursive and set on app/widgets dir to spare resources. Means hot reloading on Linux does not work... I will need to fix this.

xeruf commented 5 years ago

But you do not need to monitor the class files, only the source files. So you could put the compiled files wherever you want.

sghpjuikit commented 5 years ago

Wow, I think you may be right! It never occurred to me!

Theoretically, we do know when the compilation ended (and its ok/error status) because we know when the compilation process finishes, so class file monitoring is unneeded.

The monitoring is done to know when to reload the widgets. Right now, this is rather elegant because it monitors each widget's directory and if 1 resource or source file was modified -> schedules recompilation 2 class file was modified -> schedules widget reload. So from the point of view of the implementation all file changes are interesting and equal. Ignoring class modifications stings a bit as we lose fine grained awareness of what is going on there, but well, we don't care about that...

I'll change the implementation accordingly. Couple points:

xeruf commented 5 years ago

The Linux support is more tricky. I do not want to give up recursive monitoring on windows as its cheap and simple. I'll think of something.

https://stackoverflow.com/questions/8699293/how-to-monitor-a-complete-directory-tree-for-changes-in-linux Look at the answers below the accepted answer, they point to an interesting command.

sghpjuikit commented 5 years ago

573f918 moves us closer to separating utils. 4 classes to go.

widgets do not have to follow any convention package structure

Scratch that. I was mistaken and supporting this requires some work. Will do that later.

do not need to monitor the class files

Done.

As of 2e90808 widgets compile into app/widgets/widget/out directory so compilation separation has been taken care of. Also removes "Class already declared" warning. As a bonus: no more META-INF.

sghpjuikit commented 5 years ago

A vision of the source sets is, besides speeding up build times, to sketch out separate artifacts. This is bit of a further goal. The project structure could look like this:

The package structure of util right now is quite good and in theory, most of the packages could become their own source set/module/artifact, but that will involve additional decoupling.

sghpjuikit commented 5 years ago

Utils now do not depend on the app code. Some classes/methods were moved from/to utils. Only minimal app/util initialization logic was introduced, with no such complexity as registering behaviors as I was mentioning before, instead simplification and single responsibility principle has been applied.

@Xerus2000 do you have an idea how the project directory structure after source set separation should look like? Now is the time to discuss/get it right.

xeruf commented 5 years ago

We'll have to mull this over depending on the actual code, but this would be my first intuition:

sghpjuikit commented 5 years ago

Is there any advantage for having a resource directory? I prefer to keep it with the source code since it follows semantic grouping, not logical one, which is how I think cohesion priorities should be.

xeruf commented 5 years ago

Basically, when you assemble the application, the sources directory is compiled while the resources directory is simply copied. That once turned out to be an issue for me, where I accidentally released a little closed-source application with sourcecode inside because I had sources and resources in one. But if the size of the sources is negligible it shouldn't really matter in an open-source-project.

sghpjuikit commented 5 years ago

I think I'll leave them merged. Main-test separate. Your structure idea sounds ok to me. Is there any work you have open? Can I proceed with this? I will make a PR for this one.

xeruf commented 5 years ago

I have no open work. Also, you should probably separate util into main/test as well, whereas for demos it wouldn't make sense, i.e.

sghpjuikit commented 5 years ago

Id rather separate main-test everywhere. If tests are unneeded, the directory will not be there. Let's be consistent.

After some refactoring in progress, I'll dive into this. Finally.

sghpjuikit commented 5 years ago

Done in c3a794a. See follow-up #139