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

Update mops and call mops sources with --no-install flag #293

Merged
merged 6 commits into from
Sep 23, 2024

Conversation

ZenVoich
Copy link
Contributor

@ZenVoich ZenVoich commented Aug 2, 2024

Sometimes errors may occur when a user runsmops install because ext calls mops sources which starts with mops install, so we get two concurrent install processes. With --no-install flag this problem should go away.

@rvanasa
Copy link
Contributor

rvanasa commented Aug 2, 2024

Thanks for the fix! How recently was the --no-install flag added to Mops? Since the extension uses an externally installed Mops version (rather than the version bundled with the extension), we could adjust the error message to encourage updating Mops if the command is unsuccessful.

@rvanasa
Copy link
Contributor

rvanasa commented Aug 2, 2024

Here is what I'm imagining (updating the error message slightly below the original code change):

throw new Error(
    `Error while finding Mops packages.\nMake sure the latest version of Mops is installed locally or globally (https://mops.one/docs/install).\n${err?.message || err}`,
);

The CI error should be fixable by running npm install in case that saves you a bit of time.

@ZenVoich
Copy link
Contributor Author

ZenVoich commented Aug 5, 2024

How recently was the --no-install flag added to Mops? Since the extension uses an externally installed Mops version (rather than the version bundled with the extension), we could adjust the error message to encourage updating Mops if the command is unsuccessful.

In 0.45.0 a week ago. Maybe we should give it a time so fewer users encounter errors...

Here is what I'm imagining (updating the error message slightly below the original code change):

The CI error should be fixable by running npm install in case that saves you a bit of time.

Done

Copy link
Contributor

@rvanasa rvanasa left a comment

Choose a reason for hiding this comment

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

LGTM; let's revisit this in a few weeks.

It would be ideal if we could use a solution that also works for previous versions of Mops (or at least keeps the original behavior for previous versions), but on the other hand, it would be mostly trivial and beneficial for everyone to update to the latest version.

A possibly problematic counterexample is where an example / starter project includes a specific version of ic-mops in their dev dependencies (a somewhat common pattern), which would result in error messages for anyone using the starter project regardless of their local Mops version. For these cases, it would be good for us to support previous Mops versions if possible.

@ZenVoich
Copy link
Contributor Author

Added mops version check

@rvanasa rvanasa enabled auto-merge (squash) September 23, 2024 16:08
@rvanasa
Copy link
Contributor

rvanasa commented Sep 23, 2024

I just got a chance to check back on this. Thanks for the update! Merged.

@rvanasa rvanasa merged commit 94e8830 into dfinity:master Sep 23, 2024
3 checks passed
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.

2 participants