Skip to content

Conversation

minkyu97
Copy link
Contributor

I found that sometimes files in directories added to unmanagedResourceDirectories are ignored.

This was because the resourcesDirectories are managed as a Set like below

  def copyResourcesTask =
    Def.task {
      val t = classDirectory.value
      val dirs = resourceDirectories.value.toSet
      ...

But, you know, the default Set in scala is implemented as a HashSet, which does not guarantee ordering of elements.

For example, When I added my config directory src/main/resources/conf to unmanagedResourceDirectories in order to include a file called src/main/resources/conf/my-conf.json as a resource, but my config file was compiled as target/scala/classes/conf/my-conf.json, not target/scala/classes/my-conf.json.

When I printed out the variable dirs mentioned above, I got the following result. (For convenience, represent the project path as ~)

Set(~/src/main/resources/conf, ~/src/main/resources)

When I improved it using SortedSet and printed out the variable dirs, It looks fine!

TreeSet(~/src/main/resources,~/src/main/resources/conf)

@lightbend-cla-validator

This comment was marked as outdated.

@minkyu97

This comment was marked as outdated.

@eed3si9n
Copy link
Member

TreeSet(~/src/main/resources,~/src/main/resources/conf)

Thanks for the report and sending a PR. Are you saying that sbt currently does not support adding overlapping unmanagedResourceDirectories like src/main/resources and its subdirectory src/main/resources/conf? I think that's an intended design, and I am not sure if we should make the set sorted to support this.

@minkyu97
Copy link
Contributor Author

minkyu97 commented Mar 17, 2023

@eed3si9n Thanks for the comment.
You're right, the problem occurs when unmanagedResourceDirectories is a subdirectory of other resources.
Although this may not be mainstream, I think it's a workable way to do it and doesn't impact existing logic at all. Are there any issues you're worried about when sorting it?

@eed3si9n
Copy link
Member

Are there any issues you're worried about when sorting it?

I think so. In general having static typing in the build definition allows sbt, builds, and plugin ecosystem to evolve at different pace. I am particularly concerned about making implementation-level changes that would make semantic difference in exactly the opposite of what the type promises in an observable way. It might work for some time, but it could break again later. I would recommend creating a different directory with your conf files.

@minkyu97
Copy link
Contributor Author

minkyu97 commented Mar 17, 2023

If I move the unmanaged resources directories out of resources, the same problem will occur if one unmanaged resources directory becomes a superset of another unmanaged resources directory.

For example, the codes below might cause same problem.

Compile / unmanagedResourceDirectories += baseDirectory.value / "src/main/unmanaged_resources"
Compile / unmanagedResourceDirectories += baseDirectory.value / "src/main/unmanaged_resources/hadoop_conf"

However, I also agree with you that having static typing at build time can cause problems. After some thought I've thought that the underlying problem is not that the dirs variable is unordered, but that unordered oldBases are being passed to the rebase function. So, How about changing the sbt.io.rebase function to something like below? Do you still think it's not worth it?

  def rebase(oldBases: Iterable[File], newBase: File, zero: FileMap = transparent): FileMap =
    fold(zero, oldBases.toList.sorted)(old => rebase(old, newBase))

@minkyu97
Copy link
Contributor Author

@eed3si9n Hi, there!
I'm sorry for bothering you again 😥
Can you let me know if there is a possibility of causing issues at build time by changing copyResourcesTask as follows?

  def copyResourcesTask =
    Def.task {
      val t = classDirectory.value
      val dirs = resourceDirectories.value.toSet
      val sortedDirs = resourceDirectories.value.sorted
      val s = streams.value
      val syncDir = crossTarget.value / (prefix(configuration.value.name) + "sync")
      val factory = CacheStoreFactory(syncDir)
      val cacheStore = factory.make("copy-resource")
      val converter = fileConverter.value
      val flt: File => Option[File] = flat(t)
      val transform: File => Option[File] = (f: File) => rebase(sortedDirs, t)(f).orElse(flt(f))
      val mappings: Seq[(File, File)] = resources.value.flatMap {
        case r if !dirs(r) => transform(r).map(r -> _)
        case _             => None
      }
      s.log.debug("Copy resource mappings: " + mappings.mkString("\n\t", "\n\t", ""))
      Sync.sync(cacheStore, fileConverter = converter)(mappings)
      mappings
    }

I just added a variable, sortedDirs, and only used it in the rebase function instead of using static typing.
The directories within sortedDirs are always sorted and rebase function will traverse them in reverse order (because it is @tailrec function), so the arbitrary path X will always be processed later than X/Y which is also an arbitrary subpath of X.
Therefore, the problem will be resolved.

@eed3si9n
Copy link
Member

@minkyu97 Sorry about the delayed response. I'd be fine with going forward with the idea.

@eed3si9n eed3si9n modified the milestone: 1.9.0 Apr 24, 2023
@minkyu97
Copy link
Contributor Author

minkyu97 commented Oct 6, 2023

@eed3si9n I'm really sorry too.. I missed your comment for a long time because of my work.
I pushed a commit applying the above idea. I'd appreciate it if you could take a look at it and you can close the PR if you don't think it needed to be applied.

@minkyu97 minkyu97 force-pushed the fix-copy-resources branch from ead92f9 to d02bc01 Compare October 6, 2023 07:07
@eed3si9n eed3si9n changed the base branch from 1.9.x to 1.10.x October 8, 2023 19:53
@eed3si9n eed3si9n added this to the 1.10.0 milestone Oct 8, 2023
@eed3si9n eed3si9n merged commit 9f1d2fd into sbt:1.10.x Oct 9, 2023
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.

3 participants