Kotlin / binary-compatibility-validator

Public API management tool
Apache License 2.0
800 stars 59 forks source link

allow apiDumpDirectory outside projectDir if inside rootProjectDir #239

Open robstoll opened 3 months ago

robstoll commented 3 months ago

See https://github.com/Kotlin/binary-compatibility-validator/pull/227 this would allow that one can specify a folder outside the project folder but still within the rootProject dir.

Say you have a multi-project setup with 10 sub-projects. Currently you can only define a folder within each sub-project but not a folder in the rootProject. This would be handy if you want to have all api files in one directory.

robstoll commented 2 months ago

@fzhinkin I created an issue as you asked for in #227, how to proceed now? I did not get any feedback so far if there is a consensus that the current restriction is not needed if still inside the rootProjectDir.

qwwdfsad commented 2 months ago

FTR: @fzhinkin is taking a break, so please expect some delayed response times

robstoll commented 1 month ago

@qwwdfsad maybe you can give me pointers how to proceed? If the request as such is accepted, then I gladly add tests if needed.

fzhinkin commented 1 month ago

I agree that storing all dumps in a single directory somewhere under the rootProject's dir could be handy.

The main issue I see here from the design point of view is how all these ABI files should be stored within a single directory. For projects using a flat modules hierarchy everything is simple:

library
├── api
│   ├── core.api
│   └── extras.api
├── core
└── extras

In this case, we can continue using old file names and put everything into a single directory (library/api, or whichever name a user will choose).

However, if a project has a deeper submodule hierarchy, things are getting complicated:

library
├── api
│   ├── ????
├── core
│   ├── main
│   └── extras
└── extras

Here, it's no longer clear how generated API files should be stored within the library/api dir. Should the api directory use the same directories hierarchy as modules (i.e., library/api/extras.api, library/api/core/main.api, library/api/core/extras.api) or should it use flat files list? In the latter case, we need to distinguish library:core:extras from library:extras, so some alternative file naming scheme is required.

robstoll commented 1 month ago

I suggest we don't change the current behaviour and still dump in the subdirectory per default but allow that one can define a directory within the rootProject dir (see the mentioned PR). It is then up to the project maintainers how they store the files and what naming scheme they define.

If you prefer to change the default behaviour, then I suggest we replace : with - in the name and generate all files in one directory per default. One can still change it if they prefer another schema etc.

fzhinkin commented 1 month ago

I suggest we don't change the current behaviour and still dump in the subdirectory per default but allow that one can define a directory within the rootProject dir (see the mentioned PR). It is then up to the project maintainers how they store the files and what naming scheme they define.

Once one had configured the plugin to use a single directory within a rootProject to store dumps generated for all subprojects there, it's up to BCV how to put all the files into that directory avoiding naming conflicts along the way. Maintainers cannot control the naming scheme as BCV does not provide an extension point for that.

robstoll commented 1 month ago

ah true, my bad, they cannot modify the naming schema but modifying the directory structure would be enough to workaround name clashes.

Of course, if we want that one is able to put everything in one directory in all cases and still be able to workaround a name clash then I guess we should provide a way to modify the file name as well and use project.path as default but skip the first : and replace the remaining : with -. i.e. library:core:extras becomes core-extras and library:extras becomes extras and in case there is a library:core-extras project then we will fail for library:core:extras and point to the property which allows to manage the file name. WDYT?

fzhinkin commented 1 month ago

and in case there is a library:core-extras project then we will fail for library:core:extras and point to the property which allows to manage the file name. WDYT?

Currently, BCV could only be configured for the top level project, so there's no way to set some properties within subprojects.

We can, of course, support a top-level property allowing to configure name mapping for each subproject, but I would rather have a robust naming scheme out of the box.

robstoll commented 1 month ago

I see, so how about we use _ as replacement for : instead of -. I think - is more often used in project names than _, so with _ we will run already into less ambiguities and then we could escape _ in a project name with e.g. double __. This is still not perfect as we can see in the following:

So we need to define another escaping for either prefix or suffix in project names. I suggest we escape the suffix as I doubt that there will be a lot of cases where project names have a `` suffix. I suggest the following escaping:

WDTY?

fzhinkin commented 1 month ago

@robstoll, maybe we can simply use : as a delimiter in dump files names? Gradle's project names could not contain :, so there would be no clashes (and, well, we can just use a project path as a name's prefix).

robstoll commented 1 month ago

Personally, I wouldn't mind but Windows users would be blocked I guess because the file system doesn't allow : in file names

fzhinkin commented 1 month ago

Perhaps, we should make a separator configurable and use "_" a default one. Then, we can check for name clashes during the plugin configuration, print colliding name and force user to pick an alternative separator.

This way, we'll keep things simple, and there will always be the workaround for projects with bizarre subproject names.

robstoll commented 1 month ago

good with me, I doubt that users will have a lot of conflicts if we choose _ as default. Shall I extend my (closed) PR with this functionality or are you going to create one?

fzhinkin commented 1 month ago

Shall I extend my (closed) PR with this functionality or are you going to create one?

Yes, please, go ahead. Currently, I don't have a time budget for this particular issue, so your contributions are welcome.