scalafixOnCompile setting key #140

bjaglin · 2020-07-05T14:57:41Z

Users can control whether Scalafix should be run (with the rules defined in .scalafix.conf) as part of compile,at the project & configuration level.

The value of scalafixOnCompile is ignored when invoking directly scalafix or scalafixAll, as the CLI arguments (if there are any) take precedence over the default rules. That means for example that scalafix --check is safe to run even though scalafixOnCompile := true, as there will not be any rewrite triggered by the implicit compilation.

Enabling scalafixOnCompile wil automatically turn on caching, which can be disabled by an explicit scalafixCaching := false.

bjaglin · 2020-07-05T21:32:04Z

src/main/scala/scalafix/sbt/ScalafixPlugin.scala

+            // We can't just run scalafix from here, as it would introduce a cyclic dependency
+            // since semantic rules require compilation products. Instead we run scalafix within
+            // a custom state where compile has not been overriden.
+            val extracted = Project.extract(currentState)
+            val withOldCompile = Compat.append(
+              extracted,
+              Seq(compile.in(ref, config) := oldCompile),
+              currentState
+            )
+            extracted.runInputTask(
+              scalafix.in(ref, config),
+              input = "",
+              withOldCompile
+            )


This seems to work, but it is very hacky, as:

the state "reload" is slow

compilations for a given project/config and most annoyingly all the projects it depends on are not de-duped during the liftetime of a task execution

Still trying to come up with something better (forcing the fullClasspath value lookup to transitively depend on a non-"custom" compile).

2. compilations for a given project/config and most annoyingly all the projects it depends on are not de-duped during the liftetime of a task execution

Yes, because runInputTask is running outside of the task graph, which is dangerous for race conditions. See these references for when sbt-buildinfo used to do the same thing, and the problems it caused: sbt/sbt-buildinfo@7f3a548#diff-0a369498a5a8db3ac8fa606b544c9810

Thanks for the pointers, that confirms my concerns! How did you get rid of it there? I am just wondering if it would be applicable here.

I changed the implementation so the values were defined within the Task (monad): sbt/sbt-buildinfo@v0.7.0...v0.9.0#diff-60231996cd2e40cc694705deb78dc6d6R27; notice the case BuildInfoKey.TaskValue, which doesn't do the unsafe extracted.runTask (the case BuildInfoKey.Task is still there for backwards-compatibility and extreme cases.)

dwijnand · 2020-07-06T10:15:26Z

src/main/scala/scalafix/sbt/ScalafixPlugin.scala

+          if (runScalafixAfterCompile) {
+            // We can't just run scalafix from here, as it would introduce a cyclic dependency
+            // since semantic rules require compilation products. Instead we run scalafix within
+            // a custom state where compile has not been overriden.


Where's the cycle? val oldCompile = compile.value means "compile and store the result (CompileAnalysis) as oldCompile. So scalafix should find all the compilation products it needs in the right places in the target directories.

on master: scalafix -> fullClasspath -> compile
introduced in this PR: compile -> scalafix

OK, instead of redefining compile, try something like:

scalafix := Def.taskDyn { // ... if (runScalafixAfterCompile) scalafix.triggeredBy(compile.in(config)) else scalafix }.value

I tried that earlier but as far as I remember, it did not give me what I wanted: a failure in scalafix woudn't cascade to compile, given the fire-and-forget semantics of triggeredBy.

I see, yes, that is to be expected.

So, thinking back, why does scalafix need fullClasspath but compile doesn't? It looks like (ignoring a bunch of noise and detail) compile is defined in terms of dependencyClasspath and classDirectory, and products (a dependency of fullClasspath) is defined as

def makeProducts: Initialize[Task[Seq[File]]] = Def.task { compile.value copyResources.value classDirectory.value :: Nil }

So I wonder if your redefinition of compile could run old compile and copyResources, and then run scalafix, having redefined scalafix as depending on dependencyClasspath and classDirectory, the last of which should have the classfiles from compile and the resources from copyResources.

having redefined scalafix as depending on dependencyClasspath and classDirectory, the last of which should have the classfiles from compile and the resources from copyResources.

By removing that dependency, the problem is that scalafix could run against stale class/semanticDB files, right?

I guess there is something to do with scalafixRunExplicitly, which can control in which direction the dependency is so it's never cyclic for a given task execution, I'll give it a try.

Another way to see it is: given scalafix will always depend on compilation, then "runScalafixAfterCompile" roughly means "redefine compile as scalafix".

I think I came up with a reasonable implementation. It is a bit more tricky than I anticipated because the real scalafix can run any rule given that it's a input key, while the scalafixOnCompile can only trigger a no-arg scalafix which uses .scalafix.conf to find rules to run. That's why I changed a bit the behavior (see last paragraph of first comment).

Is that what you had in mind?

Yeah, looks good (only fast-scanned, this late at night 😄).

Nice to know it actually worked! (famous last words...)

olafurpg · 2020-07-06T13:42:37Z

Would it be possible to achieve similar functionality using triggeredBy? I'm couldn't get the following code to work as expected but I think the code looks something like this

    scalafix.in(Compile) :=
      scalafix.in(Compile).toTask(" ").triggeredBy(compile in Compile).value,

bjaglin · 2020-07-06T15:58:40Z

Would it be possible to achieve similar functionality using triggeredBy?

@olafurpg see inline discussion: compile wouldn't fail on lint error. I naively expected this behavior for visibility, but maybe it's better that way?

olafurpg · 2020-07-06T16:03:21Z

My apologies! I missed that conversation, makes sense!

bjaglin · 2020-07-08T21:45:24Z

src/main/scala/scalafix/sbt/ScalafixPlugin.scala

-          "To run on test sources use test:scalafix or scalafixAll."
+          "For example: scalafix RemoveUnused. " +
+          "To run on test sources use test:scalafix or scalafixAll. " +
+          "When invoked directly, prior compilation will be triggered for semantic rules."


Not very proud of the "When invoked directly" wording but I am trying to get the attention of power users that would call/use scalafix input keys from another context (like another input meta input key that would aggregate scalafmt & scalafix for example). In that case, scalafixRunExplicitly will be false, so Scalafix might run on stale semanticdb/class files. I haven't found a way to work around that, but I think it's such a corner case and the current implementation is quite readable that it's not worth trying to address it.

Users can control whether scalafix (running the default rules declared in .scalafix.conf) should be run as part of `compile` at the project & configuration level. The value of scalafixOnCompile is ignored when invoking directly scalafix or scalafixAll, as the CLI arguments (if there are any) take precedence over the default rules. That means for example that `scalafix --check` is safe to run even though scalafixOnCompile := true, as there will not be any rewrite triggered by the implicit compilation.

bjaglin · 2020-07-09T06:16:46Z

src/main/scala/scalafix/sbt/ScalafixPlugin.scala

+          // passively consume compilation output without triggering compile as it can result in a cyclic dependency
+          val classpath =
+            dependencyClasspath.in(config).value.map(_.data.toPath) :+
+              classDirectory.in(config).value.toPath


I have left out copyResources since I don't see why Scalafix would need it but it might be a wrong assumption.

olafurpg

I just tested this locally and it seems to work great! Although I'm personally not a fan of "format/fix on compile", it's pretty clear that a lot of people will love this addition.

I am concerned about redefining compile even when fix on compile is disabled. I wonder how this interacts with other plugins that also override compile, for example sbt-scalafmt 🤔 Can users decide the ordering in which formatting/fixing runs?

To minimize the risk of causing unintended issues, I wonder if we should extract this setting in a separate plugin that users need to explicitly enable? I took a stab at implementing this in the diff here https://github.com/scalacenter/sbt-scalafix/compare/master...olafurpg:scalafixOnCompile?expand=1

I'm not sure which approach is better, just thinking out loud. I am totally fine with the approach in this PR with scalafixOnCompile. You have more experience using this feature so I leave the decision to you

bjaglin · 2020-07-10T11:11:30Z

I can't think of any downside redefining compile when scalafixOnCompile := false, except maybe polluting a tiny bit the inspect tree for compile?

As for what happens when scalafixOnCompile := true, I am not sure the order in which plugins are (explicitly) enabled matter, and therefore if it makes any difference to have a plugin that does not trigger automatically so that users can control ordering with other compile-hijacking plugins. sbt-scalafmt runs before compilation, so that particular plugin is not a problem.

I'd tend to stick to the current implementation for simplicity unless we have factual elements that demonstrate its shortcomings.

@dwijnand, since you are here, your thoughts on this are more than welcome!

bjaglin · 2020-07-10T11:12:22Z

@olafurpg any thoughts on a4ba496 in particular?

dwijnand · 2020-07-10T11:54:08Z

I think Sébastien said it best (somewhere): if a plugin changes the default build settings/tasks, then it should be opt-in enabled, not auto-trigger from being on the classpath. But it's not a practice that is even documented or even clearly, expressly understood between major sbt plugin authors and sbt maintainers, so it's clearly not strongly observed by plugins in the ecosystem.

So, given that, I don't have any immediate feedback to give (reason why I just +1'd). But, eventually, it might become better to make the "on compile" part opt-in.

I don't even know what the behaviour would be if you did enablePlugins(ScalafmtOnCompilePlugin, ScalafixOnCompilePlugin) (let's say the "before compilation" thing weren't true). It's probably either order of definition or alphabetic ordering, but it could even be something random-looking like the order of hashing something. I know that when sbt's Plugin were re-designed with AutoPlugin the focus was on the plugin dependencies part, in particular for SbtWeb and SbtPlay: before you could get wrong behaviour if you didn't enable correctly, afterwards the order didn't matter and triggers happened as expected. But (as I said) I don't know what the behaviour is for unrelated (technically) plugins like Scalafmt and Scalafix, and I doubt it's documented.

bjaglin · 2020-07-10T12:24:52Z

Thanks a lot for the context @dwijnand !

Since we are pre 1.0 (which maybe wasn't the case for scalafmt that took the scalafmtOnCompile road, so we are stuck to that now), I guess we should go for the separate plugin way from the start.

@olafurpg should I merge this as-is and you open a PR with your extra commits? Or you just push stuff here (I guess collaborators can do that) ?

olafurpg · 2020-07-10T12:38:06Z

Pinging @sjrd to weigh in on the discussion starting at #140 (review)

Do you think that it's acceptable/idiomatic to redefine the compile task by default as is done in the current PR or do you think it would be preferable to move this logic into a separate off-by-default plugin? I wish there was a more lightweight syntax to enable an off-by-default plugin than creating project/MyPlugin.scala, from the user's perspective it's a lot of additional ceremony compared to ThisBuild / scalafixOnCompile := true.

olafurpg · 2020-07-10T12:39:48Z

@bjaglin It's a good point that sbt-scalafmt formats before compile so there shouldn't be an integration issue. However, I suppose users would ideally prefer Scalafmt to format after Scalafix when you enable scalafix+scalafmt on compile. Pinging in @poslegm who has worked on the sbt-scalafmt plugin, I wonder if it's worth considering changing the sbt-scalafmt plugin to make it possible to format after Scalafix completes?

sjrd · 2020-07-10T12:58:33Z

@olafurpg: @dwijnand explained my position quite well with regards to modifying existing tasks and settings by default. In general, if you're modifying a task or setting that is not defined by your plugin, there's one big no no: do not make your plugin auto-triggered. Because of how plugins get automatically loaded simply because of being on the classpath (including through transitive dependencies), having auto-triggered plugins that modify existing stuff can cause surprising behaviors, sometimes difficult or impossible to disable.

That said, the way that compile is overridden here is very well done. If the config is such that fix-on-compile is not true (and that being the default), compile basically ends up being overridden by itself, which basically amounts to doing nothing. The rule above about not changing others' settings and tasks in an auto-triggered plugin does not apply.

Another big danger is that any taskDyn or settingDyn worsens a) the output of inspect tree, potentially hiding large swathes of depended on keys and b) the ability for sbt to parallelize task execution, as it needs to know the result of the dynamism before proceeding with the execution of the dynamic aspects. Again, the way that this PR overrides compile means, I believe, that the parallelism of compile will be essentially unaffected. However, scalafixing of course runs sequentially, but that's expected.

So overall, from a technical standpoint of overriding compile, I think this PR is sound and does this the correct way.

Now, you didn't ask my opinion, but I can't comment on a thread like this without giving it anyway: modifying files in a task (either input files or files produced by earlier tasks) is a very bad thing, and you should never do that, ever. The parallelism of sbt means that things can execute concurrently on already created files, causing very bad issues. If you want to modify things, use a command instead (and no, you can't execute a command from a task, not in a sound way anyway).

OK, I said it once, I will now hold my peace about this.

bjaglin · 2020-07-10T13:47:42Z

modifying files in a task (either input files or files produced by earlier tasks) is a very bad thing, and you should never do that, ever

Good point, which I guess goes beyond the scope of this PR as scalafix is already an (input) task mutating input files. However, triggering it from compile would probably expose the problem much more.

Since the scalafix input task's purpose is to transform unmanagedSources output, would it be possible to prevent races by tagging unmanagedSources and scalafix with a custom tag, enforcing at most one concurrent execution by project/config?

bjaglin · 2020-07-10T13:56:05Z

Since the scalafix input task's purpose is to transform unmanagedSources output, would it be possible to prevent races by tagging unmanagedSources and scalafix with a custom tag, enforcing at most one concurrent execution by project/config?

nevermind, that wouldn't prevent tasks depending on unmanagedSources from running concurrently with scalafix....

bjaglin · 2020-07-10T14:01:31Z

I guess we could still use Tags.customLimit to enforce that scalafix does not run concurrently with any other task from the same project/config. To be completely correct, it shouldn't run concurrently with anything at all (Tags.exclusive) since sources might be read across projects/configs, but that would really impact uncached compile invocations on multi-projects builds with scalafixOnCompile := true.

poslegm · 2020-07-10T14:08:29Z

Pinging in @poslegm who has worked on the sbt-scalafmt plugin, I wonder if it's worth considering changing the sbt-scalafmt plugin to make it possible to format after Scalafix completes?

I think it's OK to make changes in sbt-scalafmt because these are projects from the same ecosystem. But I can't figure out yet exactly how to track the scalafix finish from another plugin.

olafurpg · 2020-07-10T14:11:19Z

Thank you @sjrd for pitching in! That was helpful. With all things considered, I think it's OK to merge this PR as is. I'm not convinced it's worth complicating the installation setup with a separate ScalafixOnCompilePlugin.

I agree with you that modifying files during compile is a bad thing. I have tried to prevent people from running formatting/fixing on compile since 2016 with little luck so far 😅 As for modifying files from any task I agree in principle but I haven't yet figured out how to implement it in practice so far 🤷

olafurpg · 2020-07-10T14:16:54Z

@poslegm It's a good question. I feel like it might be possible to achieve this by defining a callback hook in sbt-scalafix that sbt-scalafmt can register.

// sbt-scalafix

val scalafixOnCompileHook = taskKey[Seq[File] => Unit]("Callback hook after Scalafix has completed fixing a file")
scalafix := {
  val filesToFix = // .. command like usual
  scalafixOnComipileHook.?.value.foreach(hook => hook(filesToFix))
}

// sbt-scalafmt

TaskKey[Seq[File] => Unit]("scalafixOnCompileHook") := { files => formatFiles(files) }

You can achieve something similar by introducing a direct dependency between the plugins but I don't think that's necessary.

Either way, feel free to ignore these thoughts. This is not blocking this PR, I'm just thinking out loud.

bjaglin · 2020-07-10T14:31:45Z

I feel like it might be possible to achieve this by defining a callback hook in sbt-scalafix that sbt-scalafmt can register.

👍 let's follow-up on implementation details in scalacenter/scalafix#1206

eed3si9n · 2020-07-10T15:00:52Z

I'm somewhat wary of compile (most called task in sbt) turning into a dynamic task:

        compile := Def.taskDyn {
          val oldCompile =
            compile.value // evaluated first, before the potential scalafix evaluation
          val runScalafixAfterCompile =
            scalafixOnCompile.value && !scalafixRunExplicitly.value
          if (runScalafixAfterCompile)
            scalafix
              .toTask("")
              .map(_ => oldCompile)
          else Def.task(oldCompile)
        }.value,

When Play needed post processing, Josh and I split compile task into compileIncremental and manipulateBytecode task, which plugins can hook into. I wonder if Scalafix can use that hook? By default:

    manipulateBytecode := compileIncremental.value,

but you could probably do:

Compile / manipulateBytecode := {
  val orig = (Compile / manipulateBytecode).value // respect other plugins
  println("post processing")
  orig
}

and here's a dynamic version of that..

lazy val condition = settingKey[Boolean]("")

condition := true
Compile / manipulateBytecode := (Def.taskDyn {
  val orig = (Compile / manipulateBytecode).value
  if (condition.value) Def.task {
    println("post processing")
    orig
  }
  else Def.task { orig }
}).value

bjaglin · 2020-07-10T20:56:37Z

Thanks for chiming-in @eed3si9n!

I'm somewhat wary of compile (most called task in sbt) turning into a dynamic task:

Could you explain why it's better to redefine (with a dynamic wrapper task ) a subtask of compile rathe than compile itself? I can understand it "blurs out" the task graph a bit less (inspect tree), but is there any runtime impact as well?

It seems to work just fine, but I didn't manage to write a proper motivation in the commit message, thus my question.

eed3si9n · 2020-07-10T23:39:20Z

@bjaglin I don't think there will be noticeable performance impact.

One potential benefit of hooking into manipulateBytecode is that it's still in the compile chain of task, which means the rest of the compileTask (https://github.com/sbt/sbt/blob/v1.3.13/main/src/main/scala/sbt/Defaults.scala#L1716-L1735) won't happen on failure, which includes writing Analysis file into the disk. After all you're making compile task fail, so that should correspond with the Analysis.

olafurpg · 2020-07-11T08:12:09Z

writing Analysis file into the disk. After all you're making compile task fail, so that should correspond with the Analysis.

In the case of Scalafix, I think it would be desirable to persist incremental compile analysis files even in case Scalafix fails. As a user, I would be surprised if a Scalafix linting error would cause sbt to re-compile the Scala sources.

sjrd · 2020-07-11T22:32:41Z

Random thought: have you considered using a triggeredBy(compile) mechanism, instead of modifying compile? This should avoid technical concerns about performance/correctness of modifying compile.

bjaglin · 2020-07-11T22:44:13Z

Random thought: have you considered using a triggeredBy(compile) mechanism, instead of modifying compile?

We have: it wouldn't fail verbosely in case of a lint error.

bjaglin · 2020-07-13T10:43:50Z

I think I will go ahead with this as-is.

Outstanding comments

#140 (comment)

modifying files in a task (either input files or files produced by earlier tasks) is a very bad thing, and you should never do that, ever. The parallelism of sbt means that things can execute concurrently on already created files, causing very bad issues.

I'll follow-up with a PR with a tentative fix for that based on #140 (comment).

#140 (comment)

I'm somewhat wary of compile (most called task in sbt) turning into a dynamic task
When Play needed post processing, Josh and I split compile task into compileIncremental and manipulateBytecode task, which plugins can hook into. I wonder if Scalafix can use that hook?

Given the change that this brings, I feel like it's still better to override compile.

#140 (comment)

I feel like it might be possible to achieve this by defining a callback hook in sbt-scalafix that sbt-scalafmt can register.

Will be handled separately, see scalacenter/scalafix#1206.

olafurpg · 2020-07-13T10:50:27Z

I agree @bjaglin. LGTM 🚀

bjaglin force-pushed the scalafixOnCompile branch 2 times, most recently from 0e81719 to eb0f7cc Compare July 5, 2020 15:18

bjaglin marked this pull request as ready for review July 5, 2020 16:18

bjaglin requested review from olafurpg and mlachkar July 5, 2020 16:18

bjaglin mentioned this pull request Jul 5, 2020

sbt-scalafix: enable caching by default? scalacenter/scalafix#1164

Closed

bjaglin commented Jul 5, 2020

View reviewed changes

bjaglin force-pushed the scalafixOnCompile branch from 126e66c to b8e35c2 Compare July 5, 2020 21:57

dwijnand reviewed Jul 6, 2020

View reviewed changes

bjaglin force-pushed the scalafixOnCompile branch from b8e35c2 to be85a8b Compare July 8, 2020 21:36

bjaglin commented Jul 8, 2020

View reviewed changes

bjaglin mentioned this pull request Jul 8, 2020

caching: don't forget previous invocations #139

Merged

github-brice-jaglin added 3 commits July 9, 2020 00:16

enable caching when scalafixOnCompile := true

a4ba496

deprecated built-in rule name

7562e24

bjaglin force-pushed the scalafixOnCompile branch from be85a8b to 7562e24 Compare July 8, 2020 22:17

bjaglin mentioned this pull request Jul 9, 2020

document scalafixOnCompile scalacenter/scalafix#1203

Merged

bjaglin commented Jul 9, 2020

View reviewed changes

bjaglin mentioned this pull request Jul 9, 2020

Refresh Syntactic/SemanticDocument between rule evaluations to better handle overlapping patches scalacenter/scalafix#1204

Open

olafurpg approved these changes Jul 10, 2020

View reviewed changes

bjaglin mentioned this pull request Jul 10, 2020

Better coordination between sbt-scalafmt & sbt-scalafix when scalaf..OnCompile := true scalacenter/scalafix#1206

Open

bjaglin force-pushed the scalafixOnCompile branch from 3702195 to ad08428 Compare July 10, 2020 23:52

bjaglin force-pushed the scalafixOnCompile branch from ad08428 to 7562e24 Compare July 11, 2020 12:30

bjaglin mentioned this pull request Jul 11, 2020

add setting key to register callbacks after Scalafix runs #147

Closed

bjaglin merged commit 11d4c3f into scalacenter:master Jul 13, 2020

taisukeoe mentioned this pull request Jul 13, 2020

add scalafixOnCompileConfig to separate .scalafix.conf for scalafix-on-compilation #148

Closed

bjaglin mentioned this pull request Jul 16, 2020

prevent concurrent modifications of sources by limiting concurrency #153

Merged

scalafixOnCompile setting key #140

scalafixOnCompile setting key #140

Conversation

bjaglin commented Jul 5, 2020 • edited Loading

bjaglin Jul 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjaglin Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg commented Jul 6, 2020

bjaglin commented Jul 6, 2020

olafurpg commented Jul 6, 2020

bjaglin Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

bjaglin Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

bjaglin commented Jul 10, 2020 • edited Loading

bjaglin commented Jul 10, 2020

dwijnand commented Jul 10, 2020 • edited Loading

bjaglin commented Jul 10, 2020 • edited Loading

olafurpg commented Jul 10, 2020

olafurpg commented Jul 10, 2020

sjrd commented Jul 10, 2020

bjaglin commented Jul 10, 2020 • edited Loading

bjaglin commented Jul 10, 2020 • edited Loading

bjaglin commented Jul 10, 2020

poslegm commented Jul 10, 2020

olafurpg commented Jul 10, 2020

olafurpg commented Jul 10, 2020

bjaglin commented Jul 10, 2020

eed3si9n commented Jul 10, 2020 • edited Loading

bjaglin commented Jul 10, 2020 • edited Loading

eed3si9n commented Jul 10, 2020 • edited Loading

olafurpg commented Jul 11, 2020

sjrd commented Jul 11, 2020

bjaglin commented Jul 11, 2020

bjaglin commented Jul 13, 2020

Outstanding comments

olafurpg commented Jul 13, 2020

bjaglin commented Jul 5, 2020 •

edited

Loading

bjaglin Jul 5, 2020 •

edited

Loading

bjaglin Jul 8, 2020 •

edited

Loading

bjaglin Jul 8, 2020 •

edited

Loading

bjaglin Jul 9, 2020 •

edited

Loading

bjaglin commented Jul 10, 2020 •

edited

Loading

dwijnand commented Jul 10, 2020 •

edited

Loading

bjaglin commented Jul 10, 2020 •

edited

Loading

bjaglin commented Jul 10, 2020 •

edited

Loading

bjaglin commented Jul 10, 2020 •

edited

Loading

eed3si9n commented Jul 10, 2020 •

edited

Loading

bjaglin commented Jul 10, 2020 •

edited

Loading

eed3si9n commented Jul 10, 2020 •

edited

Loading