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

the opam lock command exits with a non-zero exit code when dependencies are not yet installed in the current switch #6341

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

progressive-galib
Copy link

fixed #5520

Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

thanks for your contribution. Aside from the two individual comments, i can note that the if clause should change from as its return value isn't used in the true case anymore. Could you change the shape of the if to the following?:

if ... then
   error_and_exit... ;
OpamPackage.Set.add nv acc

src/client/opamLockCommand.ml Outdated Show resolved Hide resolved
Comment on lines 101 to 102
"Skipping %s, dependencies are not satisfied in this switch, \
not installed packages are:\n%s"
Copy link
Member

Choose a reason for hiding this comment

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

The error message doesn't correspond to the action anymore (skipping vs. aborting)

Copy link
Author

@progressive-galib progressive-galib Dec 19, 2024

Choose a reason for hiding this comment

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

done but my new error msg might not make any sense grammatically,

as English is not first language help a bit, pls.

@kit-ty-kate kit-ty-kate added this to the 2.4.0~alpha1 milestone Dec 19, 2024
@progressive-galib

This comment was marked as outdated.

@progressive-galib
Copy link
Author

progressive-galib commented Dec 19, 2024

my new error msg might not make any sense grammatically,

as English is not first language help a bit, pls.

the text below is the texts of last commits that have been cleaned up and squashed.

 sorry 1st commit was a mistake

Update src/client/opamLockCommand.ml based on suggestions of

@kit-ty-kate

Co-authored-by: Kate <[email protected]>

i commited last time  from ipad and sorry for the mistale

my error msg needs correction

all done

all done, this time really
@progressive-galib progressive-galib force-pushed the galib-lock-exit-error-i5520 branch from abdb2b1 to a658c68 Compare December 20, 2024 07:13
@progressive-galib
Copy link
Author

@kit-ty-kate done merge please

also can i talk to you about something else here or if you could msg me somewhere else ? i want to contribute more to opam more, i am just getting started so if you could suggest some easy/small issues and walk me through the source tree/code structure that would be nice

@progressive-galib
Copy link
Author

@rjbou @kit-ty-kate any update ?

@kit-ty-kate
Copy link
Member

Hi, sorry for the wait.
First of all, while i know English isn't your first language, i'd like to invite you to use a less pushy language when requesting action from us maintainers here and elsewhere, even more when requesting action during the winter break when the change isn't critical.

Regarding your request for discussion, both @rjbou and i have our email addresses displayed on our github profile, feel free to send us an email.

Regarding this here PR, i don't think the new behaviour matches the requested behaviour in #5520. I'm not sure it makes sense to stop as soon as a missing dependency is detected. Maybe keeping the same error message as before but simply returning the fact that an error occurred from this function, then use it to call OpamStd.Sys.exit_because if there was an error at the end of OpamCommands.lock.lock might be a better behaviour. This way the behaviour for (non-script) users of opam lock does not change while allowing script users to make sure the dependencies are all installed.
What do you think? As a user, how would you expect opam lock to work?

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.

opam lock does not exit with an error code after displaying an error
3 participants