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

feat(pkg): Remove opam-repository-url option #9373

Conversation

Leonidas-from-XIV
Copy link
Collaborator

Since the repositories are specified in the workspace file where more information like URL, branch or tag can be stored this makes for a better set-up.

Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

LGTM. I think you can remove the opam repository path option as well.

@Leonidas-from-XIV
Copy link
Collaborator Author

I think you can remove the opam repository path option as well.

I tried but that will currently leave us without a way to specify directories as opam-repos, as the dune-workspace currently only supports git repos. So maybe the way forward would be to allow that first and then remove that option?

@rgrinberg
Copy link
Member

So maybe the way forward would be to allow that first and then remove that option?

I was under the impression that it was already allowed with file://. If not, then I completely agree that we need a way valid replacement before removing it.

@Leonidas-from-XIV Leonidas-from-XIV force-pushed the remove-opam-repository-url-cli-arg branch from 0c28f48 to cb76424 Compare December 5, 2023 15:49
@Leonidas-from-XIV
Copy link
Collaborator Author

Ah, I forgot that that actually works. I've updated the PR, but kept the two commits separate, since the latter is a change that affects a lot of tests.

@Leonidas-from-XIV Leonidas-from-XIV force-pushed the remove-opam-repository-url-cli-arg branch from cb76424 to 7ba9d7d Compare December 5, 2023 17:11
@Leonidas-from-XIV
Copy link
Collaborator Author

Ok, also removed the opam-repository-path, but that required a lot of changes and modifying dune-workspace with Unix tools is quite painful.

@Alizter
Copy link
Collaborator

Alizter commented Dec 5, 2023

In #9343 I am removing all the --context arguments and replacing them with positional lock directories. Maybe we should hold off on the opam-repository-path changes until thats done, since it will simplify the current workspace wrangling.

@rgrinberg
Copy link
Member

I'll leave it to Marek to decide, but I would suggest not to delay merging this PR for other work. There's some work I'd like to do that is waiting for this PR to be merged first.

Since the repositories are specified in the workspace file where more
information like URL, branch or tag can be stored this makes for a
better set-up.

Signed-off-by: Marek Kubica <[email protected]>
@Leonidas-from-XIV Leonidas-from-XIV force-pushed the remove-opam-repository-url-cli-arg branch from 7ba9d7d to 52a49e8 Compare December 6, 2023 09:44
@Leonidas-from-XIV Leonidas-from-XIV force-pushed the remove-opam-repository-url-cli-arg branch from 52a49e8 to 2c638d0 Compare December 6, 2023 10:42
@Leonidas-from-XIV Leonidas-from-XIV merged commit 21f64f1 into ocaml:main Dec 6, 2023
@Leonidas-from-XIV Leonidas-from-XIV deleted the remove-opam-repository-url-cli-arg branch December 6, 2023 11:27
@Leonidas-from-XIV
Copy link
Collaborator Author

I've merged this because at the end of the day, someone will have to rebase (and I know it sucks, I just did and more tests broke). Like Rudi I also have some work that is blocked on this, so if merging this unblocks both of us that's some compromise.

Changing the CLI is always painful because it touches a lot of tests. Though I assume this will get better once we have a post-1.0 CLI interface.

@Alizter
Copy link
Collaborator

Alizter commented Dec 7, 2023

@Leonidas-from-XIV FTR I wasn't complaining about rebasing, but rather suggesting that it would have been easier for you. Everything in #9343 is already rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants