Update PublishModule.scala to additional check for MAVEN_LOCAL_REPO #4270
Conversation
previously, we only took the MAVEN_LOCAL_REPO directory without appending `/ "repository"`. So the original version was `~/.m2/repository` and the previous would only be `/some/path/to/.m2`. This adds the proper sub-directory.
|
This looks fine. @imdaveho do you have a link to any reference about what |
| sys.props.get("maven.repo.local").map(os.Path(_)) | ||
| .getOrElse(sys.env.get("MAVEN_LOCAL_REPO").map(os.Path(_)) | ||
| .getOrElse(os.Path(os.home / ".m2", T.workspace))) / "repository" |
There was a problem hiding this comment.
This needs to be put into a dedicated Tast.Input, otherwise cache-invalidation won't trigger when one of the variables change.
Once, we have that, we can reuse it for the default of publishM2Local to.
There was a problem hiding this comment.
Do you mean a Task.Input? I'm not sure how to do that (fairly new to this). Is it just creating a new function that wraps this portion so that this function can call that function?
As I mentioned regarding conventions, this is more of a preference issue not a functional one.
I'm OK with closing this, since I'm not seeing official documentation on the exact environment variable I used. I am seeing references to setting the property with MAVEN/JAVA_OPTS. If that is the case, I would remove the environment variable and focus on adding the properties.
I don't know what you mean about Task.Input. I don't see this as part of the Mill API.
Update:
nvmd, I see it now (just had to click into the Task type in the docs). Will try to update the branch based on my interpretation of what you said.
Update 2:
ok, don't know how to do this without spending more time understanding what each task/function is trying to do and grokking the internals of mill and how scala works (newbie).
No rush to merge, and totally fine to close the PR since I don't believe I'll get to this any time soon.
Update 3:
ok, tried something. Let me know if it makes sense
again, this might be against jvm conventions, but I did not find definitive references in the maven or ivy project pages, but there are several references in unofficial pages:
I would say reject this PR and the other one (#4269) if this isn't convention to use environment variables, and rather to pass in properties using JAVA_OPTS or MAVEN_OPTS or through -D using something like .mill_opts. |
This reverts commit f3fdd13. Did not mean to include other files than PublishModule.scala
| publishM2LocalTask(getM2LocalRepoPath)() | ||
| } | ||
|
|
||
| def getM2LocalRepoPath: Task[os.Path] = Task.Input { |
There was a problem hiding this comment.
I'd rather name this m2LocalRepoPath
| os.Path(m2RepoPath, T.workspace) | ||
| })() | ||
| } | ||
| def publishM2Local(m2RepoPath: String = getM2LocalRepoPath.toString()): Command[Seq[PathRef]] = |
There was a problem hiding this comment.
This doesn't look right. Since getM2LocalRepoPath is a task, the toString return it's task-name instead of the actual path.
There was a problem hiding this comment.
Oh, this is already merged. Either I read the code wrong or we have no proper test coverage, otherwise we should see some test failures.
lefou
added
the
post-review-required
lefou
removed
the
post-review-required
…om-lihaoyi#4270) This was referenced in com-lihaoyi#4269. This checks for m2 (Maven)
This was referenced in #4269. This checks for m2 (Maven)