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

scan for projects if --project-dirs is defined #1093

Closed

Conversation

robstoll
Copy link
Contributor

@codecov
Copy link

codecov bot commented Nov 15, 2019

Codecov Report

Merging #1093 into master will decrease coverage by 0.19%.
The diff coverage is 58.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1093     +/-   ##
=========================================
- Coverage      68%   67.81%   -0.2%     
=========================================
  Files          93       93             
  Lines        1466     1510     +44     
  Branches       43       40      -3     
=========================================
+ Hits          997     1024     +27     
- Misses        469      486     +17
Impacted Files Coverage Δ
.../scala/org/scalasteward/core/application/Cli.scala 94.11% <ø> (ø) ⬆️
...ala/org/scalasteward/core/application/Config.scala 0% <0%> (ø) ⬆️
...org/scalasteward/core/repocache/RepoCacheAlg.scala 0% <0%> (ø) ⬆️
...ala/org/scalasteward/core/nurture/NurtureAlg.scala 7.82% <0%> (-0.92%) ⬇️
...la/org/scalasteward/core/repocache/RepoCache.scala 100% <100%> (ø) ⬆️
...a/org/scalasteward/core/scalafmt/ScalafmtAlg.scala 100% <100%> (ø) ⬆️
.../main/scala/org/scalasteward/core/sbt/SbtAlg.scala 64.17% <55%> (-1.16%) ⬇️
.../scala/org/scalasteward/core/io/WorkspaceAlg.scala 84.44% <94.59%> (+46.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78ee4d5...8472690. Read the comment docs.

@robstoll robstoll force-pushed the other-project-layout branch 3 times, most recently from 0f20761 to 2824097 Compare November 15, 2019 16:13
@robstoll
Copy link
Contributor Author

@fthomas personally I don't like many case p if ... statements, but I can switch from
if/else to case if you prefer it.

@robstoll robstoll force-pushed the other-project-layout branch 2 times, most recently from 890e706 to 2b3c6cc Compare November 15, 2019 20:44
@fthomas
Copy link
Member

fthomas commented Nov 15, 2019

Hi @robstoll, many thanks for the PR. Can you provide an overview what this PR does and what the overall goal is? This PR touches a lot of code and it should be easier to review if the intention behind the changes are known.

@robstoll
Copy link
Contributor Author

robstoll commented Nov 15, 2019

@fthomas the overall goal is to allow that one can use scala-steward in a project which

  • has the build.sbt not in the root (e.g. in app/build.sbt)
  • has multiple sbt projects within the same repo

In this sense it should solve #46 and #678 IMO. The functionality is kind of documented in Cli and in WorkspaceAlgTest

And I should mention that I was tempted to remove RepoCache.maybeSbtVersion/maybeScalafmtVersion they aren't used at all. In case you don't see a use case for it, it would probably make sense to remove them instead of introducing projectPathToSbtVersion and projectPathToSbtVersion

@robstoll robstoll force-pushed the other-project-layout branch 2 times, most recently from cffec39 to 2f8e877 Compare November 15, 2019 21:18
@robstoll
Copy link
Contributor Author

robstoll commented Jan 4, 2020

@fthomas any comments/suggestion for improvements on this one?

@mzuehlke
Copy link
Member

mzuehlke commented Jan 6, 2020

Hi @robstoll
I see the need to support projects which are not following the default layout and as you mention this would solve multiple open issues.

But in my opinion a global setting is not very practical, unless you set it to ** or you would have to update it often (especially fro the public bot instance). But if you introduce a local flag in .scala-steward.conf you don't need all the fancy globbing logic:

# specify the root(s) of the projects. In this directory a "build.sbt" file is expected
# Default: './' (The root of the repository)
projects.roots = ['/app']

@robstoll
Copy link
Contributor Author

robstoll commented Jan 6, 2020

Would be fine with me as well but I am not changing things unless I get green lights for idea as such from a collaborator including feedback in case something else needs to be done.

@jan-pieter
Copy link

A previous PR suggests using .scala-steward.conf for this type of configuration: #688. For me, both ways would work because we are self-hosting scala steward. My personal opinion is that this is best done using .scala-steward.conf because that would make it configurable for open source repo's without changing the flags in this repository.

@robstoll
Copy link
Contributor Author

robstoll commented Jan 7, 2020

Not changing the configuration of this repository makes a lot of sense IMO. So let's move to .scala-steward.conf once @fthomas gives green lights for this feature as such and provides additional feedback

@robstoll
Copy link
Contributor Author

@fthomas would you mind to give feedback about the general idea of supporting non-default project layouts? #1382 deals with one aspect of it as well, so there is definitely some need for it. I would still update this PR to move the config to .scala-steward.conf

@alexklibisz
Copy link
Contributor

Hi, are there any updates on this? We're getting a lot of value out of scala-steward and would really like it for a few repos which have sbt projects in sub-directories. Is it a matter of getting the conflicts resolved? If so I can take a pass at it. Or is it a matter of more substantial changes which would take longer to resolve?

@albertpastrana
Copy link

Hi @robstoll and @fthomas, thanks your work on this issue.

I see this PR has been sitting here for a while, given that we are very interested in this feature, is there anything we can do to get it done?

Happy to help if needed!

@robstoll
Copy link
Contributor Author

@albertpastrana thomas or another contributor with merge rights needs to get active. See my last two posts.
I am using this PR in production for almost a year and it works great.

@albertpastrana
Copy link

Thanks a lot for the info @robstoll!!

@fthomas
Copy link
Member

fthomas commented Sep 14, 2020

I took a look at this PR and #1382 again now. A lot has changed since November last year - for example, we now support multiple build tools; sbt and scalafmt versions are not explicitly in RepoCache anymore; etc. - but I think the goal is now clear to me and there is a solution that would go well with the current implementation.

Until now there is 1:1 relation between a Repo and the root directory of sbt, Maven, or Mill builds. But some repos do not contain the build files in the root directory of the repository or contain multiple builds in different directories. So we want to allow a 1:n relation between a Repo and build roots.

I would start with adding a new data type for build roots (lets call it BuildRoot) which contains a Repo and the relative path to a build root in the repository. To keep things as simple as possible we should only need to go from a Repo to a BuildRoot when we're interacting with the build system so that we can query multiple builds for dependencies but in all other places we ignore from which BuildRoot a dependency is coming from.

The entry point for interactions with build systems is BuildToolDispatcher. I think most changes need to happen here. BuildToolDispatcher could use RepoConfigAlg to get the RepoConfig, which need to have a new config for build roots, and construct the user-specified BuildRoots. For each BuildRoot it then does what it is currently doing for the root directory and collects the results.

BuildToolDispatcher and the build tool traits like SbtAlg inherit from BuildToolAlg which uses Repo for its methods. Since SbtAlg, MavenAlg, and MillAlg should now operate on BuildRoots instead of Repo but BuildToolDispatcher should still use Repo we need to add a new type parameter to the BuildToolAlg, i.e. BuildToolAlg[F, R] where R would be either a Repo or a BuildRoot.

SbtAlg, MavenAlg, and MillAlg then need to be changed to use BuildRoot instead of Repo. A new buildRootDir in WorkspaceAlg is probably handy for that. These changes are rather mechanical.

I think these are all necessary changes to support repos with the build in a subdirectory or multiple builds in different subdirectories.

@robstoll
Copy link
Contributor Author

robstoll commented Sep 15, 2020

I don't have time to work on this right now, so if some else would like to take over (or start over again), go on. Maybe it makes sense that @bytecodeguru addresses the multiple files aspect in his PR as this one is at least rebased on master. I'll take a look in December again, if there was no action going on here or in #1382 then I'll might consider to do it myself.

@fthomas
Copy link
Member

fthomas commented Jan 15, 2021

Superseded by #1875.

@fthomas fthomas closed this Jan 15, 2021
@robstoll robstoll deleted the other-project-layout branch January 15, 2021 20:46
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.

Support nonstandard project layout.
6 participants