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

Check for collisions in Cross instances, ban forward slash in Cross segments, support os.SubPath in Cross modules #2836

Closed

Conversation

jackkoenig
Copy link

@jackkoenig jackkoenig commented Oct 9, 2023

Fixes #2835

This PR does 3 things (and I'm happy to split it into smaller PRs since each commit is separable):

  1. Checks for collisions in cross instances
  2. Bans '/' in cross segments (my original approach is now disallowed, and the user is encouraged to use os.SubPath)
  3. Adds support for os.SubPath in Cross instances

My implementations for (1) and (2) could be better, I don't love throwing exceptions, but I wanted to get something functional out for review to get pointers on how better to report errors to the user here. It would be nice to print the actual line rather than just printing the filename and line.

For (3), I have not yet added a test because I figure it would make sense to include in the examples, probably in a section immediately after https://mill-build.com/mill/Cross_Builds.html#_dynamic_cross_modules (ie. before Use Case: Static Blog), but I wanted @lefou's blessing before starting that work.

@lefou
Copy link
Member

lefou commented Oct 10, 2023

In #2835 (comment) I wrote:

I don't think allowing a cross-value to open up multiple sub-paths is the right solution. We already map n cross-values to n sub-directories. Now, when we also allow the value itself to contain sub-directories, we can't no longer guarantee unique T.dest directories.

Here is my counter example, in which both cross module instances whould use the same T.dest path for their targets.

import mill._
object foo extends Cross[Foo](("a", "b/c"), ("a/b", "c"))
trait Foo extends Cross.Module2[String, String] {
  ...
}

Instead, we could encode the special / character in the path. But that would create less nice cache directory names.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change it to os.SubPath to avoid escaping the base path with ... Also, as discussed in #2835, we need to check the given cross instances for collisions (at runtime).

@jackkoenig jackkoenig changed the title Handle forward slashes in cross modules Check for collisions in Cross instances, ban forward slash in Cross segments, support os.SubPath in Cross modules Oct 13, 2023
@jackkoenig
Copy link
Author

@lefou I ended up taking a slightly different approach, rather than using os.SubPath when processing segments, I banned / in segments and instead encourage the user to use os.SubPath themselves. See the 1st PR comment up top where I've described the new approach in detail.

@lefou
Copy link
Member

lefou commented Oct 18, 2023

I'm not convinced. Why we want to force a path on things that aren't paths, like a version matrix? As we agreed that we want cross module parameters to be able to take more that one segment, why should we only allow that for paths, but not other values? Instead we should encourage to model a build using domain types when needed. This includes avoiding generic data types like strings and paths in favor to more specific types, hence the type class to still be able to map to a segmented path.

IMHO, the essential thing is to make sure, there are no collisions, so we have safe cache locations and unambiguous CLI arguments.

@lefou
Copy link
Member

lefou commented Jan 23, 2024

Superseded by #2984

@lefou lefou closed this Jan 23, 2024
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.

Mill cannot handle forward slash in crossValue
2 participants