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

Fix non-interactive behavior for opam-monorepo depext #216

Merged
merged 1 commit into from
Oct 22, 2021

Conversation

samoht
Copy link
Collaborator

@samoht samoht commented Oct 21, 2021

Fixes #212

Copy link
Contributor

@NathanReb NathanReb 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 good! It seems right to ask for confirmation before installing packages.

Does the opam mention of the to be run sudo command appears when running this? If not it might be worth also mentioning this in the prompt.

Could you explain how that fixes #212 though as it isn't entirely clear to me where the bug was coming from.

Besides that I have a couple comments and we need a changelog entry!

cli/depext.ml Outdated Show resolved Hide resolved
cli/prompt.ml Outdated Show resolved Hide resolved
@samoht
Copy link
Collaborator Author

samoht commented Oct 22, 2021

Could you explain how that fixes #212 though as it isn't entirely clear to me where the bug was coming from.

It seems that OpamSysInteract.install is rather brutal and fail if there is any interactive prompt (as reported on #212). This PR makes handle the prompt part directory in opam-repository and set the right flag in opam-lib's state. By saying this I realise that I forgot to do this... I'm adding this now. Seems that I've got confused because I tested it on brew which doesn't have interactive prompts...

@samoht
Copy link
Collaborator Author

samoht commented Oct 22, 2021

I've tried to address all your comments. When you are happy with the changes I'll squash my commits.

Copy link
Contributor

@NathanReb NathanReb 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 great now, thanks!

We can merge once the Sys.Break thing is removed!

@samoht
Copy link
Collaborator Author

samoht commented Oct 22, 2021

We can merge once the Sys.Break thing is removed!

So you want me to continue to see ugly backtraces? :p I'll revert this for now, but I think it's a common behaviour with interactive scripts.

@samoht samoht changed the title Fix interactive behavior for opam-monorepo depext Fix non-interactive behavior for opam-monorepo depext Oct 22, 2021
@samoht
Copy link
Collaborator Author

samoht commented Oct 22, 2021

Rebased and squashed.

@NathanReb NathanReb merged commit 61aa34e into tarides:main Oct 22, 2021
@samoht samoht deleted the fix-depext branch October 22, 2021 14:26
NathanReb added a commit to NathanReb/opam-repository that referenced this pull request Nov 23, 2021
CHANGES:

### Added

- Add a list subcommand to list the duniverse packages in the lockfile
  (tarides/opam-monorepo#217, @samoht)

### Changed

- Only warn users about missing dune-ports repo in OPAM switch if no solution
  can be found due to packages not building with dune (tarides/opam-monorepo#210, @Leonidas-from-XIV)
- Rename the `--repo` option to `--root` to make it more
  straightforward  that this is referring to the project root (tarides/opam-monorepo#218, @samoht)
- Improve the wording of the lockfile selection log (tarides/opam-monorepo#222, @NathanReb)
- Display the full solver error with `--verbose` (tarides/opam-monorepo#229, @emillon)

### Fixed

- Better errors for `opam-monorepo depext`, especially for non-interactive
  shells (tarides/opam-monorepo#216, @samoht)
- Properly detect all opam packages defined in the current repository, preventing it
  from later pulling duplicates into the duniverse if they were part of the target packages
  dependencies. (tarides/opam-monorepo#203, @Leonidas-from-XIV)
- Properly report missing dune-project file when trying to determine the
  to-be-genrated lockfile name (tarides/opam-monorepo#227, @NathanReb)
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.

depext without -y displays an error
2 participants