Skip to content

Move the collectionContrib project to a collection-contrib subdirectory #63

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

Closed
wants to merge 2 commits into from

Conversation

julienrf
Copy link
Collaborator

This is a tentative to address #51

At least now, the package task builds a .jar file that contains .class files. The publishLocal task, however, still does not build a .jar file containing the .class files.

I’ve noticed that collectionContribJS/publishLocal builds a correct .jar file. The only difference between the JS and JVM projects is that the JVM project applies the scalaModuleSettingsJVM settings, which does something with sbt-osgi, which I didn’t dig into…

@julienrf julienrf requested a review from SethTisue October 28, 2019 08:46
@julienrf
Copy link
Collaborator Author

Also, I’ve noticed that the directory layout of the generated artifact is unusual: it doesn’t contain a jars directory. Instead, there is a bundle directory (that contains this .jar file where the .class files are missing). I’m not sure whether this is expected or not.

@SethTisue
Copy link
Member

(haven't gotten to this yet, but haven't forgotten about it)

@mpollmeier
Copy link

Doesn't look like this fixes the publishing issue. The two jars published by publishLocal still only contain metadata.

Note however that package creates three jars:

  • target/scala-2.13/scala-collection-contrib_2.13-....jar which only contains metadata (that's what's being published)
  • .jvm/target/scala-2.13/scala-collection-contrib_2.13-....jar which looks correct
  • .js/target/scala-2.13/scala-collection-contrib_sjs....jar which.

Maybe this helps in getting to the bottom of it.

mpollmeier added a commit to mpollmeier/scala-collection-contrib that referenced this pull request Nov 25, 2019
addresses scala#51

other attempt doesn't fully work, see scala#63 (comment)
mpollmeier added a commit to mpollmeier/scala-collection-contrib that referenced this pull request Nov 25, 2019
addresses scala#51

other attempt doesn't fully work, see scala#63 (comment)
@julienrf
Copy link
Collaborator Author

Since this is involving sbt-crossproject, I’m pinging @sjrd, maybe the cause of the problem is obvious to him. See also the issue discussion in #51.

@sjrd
Copy link
Member

sjrd commented Nov 26, 2019

I think you're simply publishing the implicit root project instead of (or more likely in addition to) the collectionContrib and collectionContribJS subprojects. One of the great caveats of aggregation in sbt, and in particular of aggregation in the root project.

You shouldn't use a naked publishSigned, but rather individually invoke collectionContrib/publishSigned and collectionContrib/publishSigned.

This doesn't have a direct relationship to sbt-crossproject, but of course due to how sbt-crossproject works, you end up with an implicit root project that you would otherwise not have.

I'll conclude by saying that the changes in this PR are not necessary nor helping. It is purely a matter of what commands are invoked when publishing.

@julienrf
Copy link
Collaborator Author

We do scope the publish command to a specific project:

projectPrefix="collectionContribJS"

@SethTisue SethTisue self-assigned this Dec 15, 2019
@SethTisue
Copy link
Member

SethTisue commented Dec 15, 2019

the changes in this PR are not necessary nor helping

that's how it looks to me as well. I think this an sbt-osgi thing, most likely. I'm closing the PR and looking into other fixes for #51

@julienrf
Copy link
Collaborator Author

julienrf commented Jan 1, 2020

This seems to fix #64.

@julienrf julienrf reopened this Jan 1, 2020
This is a tentative to address #51

At least now, the `package` task builds a .jar file that contains .class files. The `publishLocal` task, however, still does not build a .jar file containing the .class files.

I’ve noticed that `collectionContribJS/publishLocal` builds a correct .jar file. The only difference between the JS and JVM projects is that the JVM project applies the `scalaModuleSettingsJVM` settings, which does something with sbt-osgi, which I didn’t dig into…
@julienrf julienrf force-pushed the fix-project-directory-layout branch from 09474af to eb93d21 Compare January 1, 2020 13:57
@SethTisue
Copy link
Member

hmm, interesting. I'll take another look

@SethTisue
Copy link
Member

SethTisue commented Feb 15, 2020

I poked at this for an hour or so just now. I was able to reproduce #64, and I also saw that this PR fixes #64 in the narrow sense that the JUnit tests don't have red squigglies. But it also didn't get me a fully functioning project in IntelliJ. I'm not able to run the tests, or even jump-to-definition from src/test to src/main.

So I'm a bit reluctant to merge this PR since it sort of fixes things and sort of doesn't, and adds an extra level of directories that seems like it should be unnecessary, and that we don't have in other cross-built Scala modules such as scala-parser-combinators.

But I feel that I've poked at this long enough now, and I didn't reach any firm conclusions, so I won't stand in the way if you want to merge this, Julien. But I'm curious whether this PR actually makes you able to compile and run the tests? I'm not sure it's worth merging this unless it gets at least some of us to where IntelliJ works as it should.

@julienrf
Copy link
Collaborator Author

@SethTisue Thanks for your time! I don’t remember if I could run the tests from within IntelliJ, I probably didn’t test that. I would say that anyway the problem is on IntelliJ side. Let’s not spend more time on it.

@julienrf julienrf closed this Feb 17, 2020
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.

4 participants