Skip to content

Added monocle. #11468

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

Merged
merged 5 commits into from
Jul 16, 2021
Merged

Added monocle. #11468

merged 5 commits into from
Jul 16, 2021

Conversation

dotbg
Copy link
Contributor

@dotbg dotbg commented Feb 18, 2021

No description provided.

Copy link
Contributor

@griggt griggt left a comment

Choose a reason for hiding this comment

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

It looks like this PR is accidentally reverting many of the other community build project submodules to older commit hashes, which is one of the reasons that the CI is failing.

To fix, you should probably run git submodule update --init and then git add those submodules to your commit so they are back on par with current master.

The CI will probably still fail after updating the submodules, but this will help isolate the actual changes needed to get this PR working.

@dotbg
Copy link
Contributor Author

dotbg commented Feb 19, 2021

It looks like this PR is accidentally reverting many of the other community build project submodules to older commit hashes, which is one of the reasons that the CI is failing.

To fix, you should probably run git submodule update --init and then git add those submodules to your commit so they are back on par with current master.

The CI will probably still fail after updating the submodules, but this will help isolate the actual changes needed to get this PR working.

Thanks a lot. It will save me quite some time.
Is there any way to check the versions? I am not used to git submodules (yet)

@smarter
Copy link
Member

smarter commented Feb 23, 2021

This currently fail with:

Test dotty.communitybuild.CommunityBuildTestA.monocle failed: java.lang.RuntimeException: Publish command is not specified for shapeless

val shapeless needs to be modified to add sbtPublishCommand, probably with just = "publishLocal".
You can run this test locally from sbt with:

community-build/testOnly dotty.communitybuild.CommunityBuildTestA -- *monocle

@griggt
Copy link
Contributor

griggt commented Feb 23, 2021

I'm not sure publishing shapeless is going to fix much here, as the build is trying to use shapeless 2 and not shapeless 3.

Looking at the upstream Monocle upgrade-3-RC1 branch, it appears that only a subset of the subprojects are able to be built using Scala 3, not all of them, as is being attempted in this PR. (generic and example, the ones which use shapeless, do not appear to have been upgraded to Scala 3)

Also, I believe this PR originally had the submodule pointed to a commit hash on the upstream master branch (8eea9f478ae1aef613550fbb4647c636d8bcfd09) , rather than upgrade-3-RC1, but it seems like the Monocle submodule is now missing from the PR entirely.

@griggt
Copy link
Contributor

griggt commented Feb 23, 2021

Some suggestions:

To add the Monocle submodule:

cd community-build/community-projects/Monocle
git fetch
git checkout upgrade-3-RC1
cd ..
git add Monocle
git commit

To adjust what's being attempted to be built/tested: modify sbtTestCommand for monocle in projects.scala to something like:

    sbtTestCommand = "coreJVM/test; lawJVM/test; ... etc ...",

@dotbg
Copy link
Contributor Author

dotbg commented Feb 23, 2021

I'm not sure publishing shapeless is going to fix much here, as the build is trying to use shapeless 2 and not shapeless 3.

Looking at the upstream Monocle upgrade-3-RC1 branch, it appears that only a subset of the subprojects are able to be built using Scala 3, not all of them, as is being attempted in this PR. (generic and example, the ones which use shapeless, do not appear to have been upgraded to Scala 3)

Also, I believe this PR originally had the submodule pointed to a commit hash on the upstream master branch (8eea9f478ae1aef613550fbb4647c636d8bcfd09) , rather than upgrade-3-RC1, but it seems like the Monocle submodule is now missing from the PR entirely.

I will check whether the most recent shapeless could be used. thanks for giving a hint

@dotbg dotbg force-pushed the added-monocle branch 3 times, most recently from 88d043d to 051a4ee Compare February 24, 2021 19:09
@dotbg dotbg marked this pull request as ready for review March 5, 2021 21:11
@dotbg dotbg marked this pull request as draft March 5, 2021 21:13
@dotbg dotbg closed this Mar 5, 2021
@dotbg dotbg reopened this Mar 5, 2021
@dotbg dotbg force-pushed the added-monocle branch 3 times, most recently from 791a0f9 to a9354ca Compare April 10, 2021 23:55
@b-studios
Copy link
Contributor

Hey @dotbg, thanks for your contributions! What is the current status of this PR?

@dotbg dotbg force-pushed the added-monocle branch from a9354ca to 28b25ae Compare May 7, 2021 20:53
@dotbg
Copy link
Contributor Author

dotbg commented May 7, 2021

Hi @b-studios, I have just updated the PR. I had few issues with the build before RC2.

@dotbg dotbg force-pushed the added-monocle branch from 28b25ae to 35719d8 Compare May 7, 2021 20:56
Co-authored-by: Tom Grigg <tomegrigg@gmail.com>
@bishabosha
Copy link
Member

Hi @dotbg is this PR ready to review?

@dotbg dotbg marked this pull request as ready for review July 16, 2021 16:12
@dotbg dotbg requested a review from griggt July 16, 2021 16:14
@dotbg
Copy link
Contributor Author

dotbg commented Jul 16, 2021

Hi @dotbg is this PR ready to review?

Hi. yes, it is ready to be reviewed

@griggt griggt enabled auto-merge (squash) July 16, 2021 17:05
@griggt griggt merged commit cc47c56 into scala:master Jul 16, 2021
@dotbg dotbg deleted the added-monocle branch July 17, 2021 11:24
BarkingBad pushed a commit to BarkingBad/dotty that referenced this pull request Jul 23, 2021
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.

5 participants