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: usage of potentially wrong melos version during script execution #783

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

Conversation

bspinner
Copy link

@bspinner bspinner commented Oct 29, 2024

Description

See long comment at https://github.com/invertase/melos/pull/783/files#diff-35f09e1c67ac8d5bd4889489c6ce0f6dc320b1491828444f2eca21c0f8ff765cR343 :)

Type of Change

  • feat -- New feature (non-breaking change which adds functionality)
  • 🛠️ fix -- Bug fix (non-breaking change which fixes an issue)
  • ! -- Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 refactor -- Code refactor
  • ci -- Build configuration change
  • 📝 docs -- Documentation
  • 🗑️ chore -- Chore

Copy link

docs-page bot commented Oct 29, 2024

To view this pull requests documentation preview, visit the following URL:

docs.page/invertase/melos~783

Documentation is deployed and generated using docs.page.

@CLAassistant
Copy link

CLAassistant commented Oct 29, 2024

CLA assistant check
All committers have signed the CLA.

@bspinner bspinner changed the title Fixes use of potentially wrong melos version during script execution Fix: usage of potentially wrong melos version during script execution Oct 29, 2024
@bspinner bspinner changed the title Fix: usage of potentially wrong melos version during script execution fix: usage of potentially wrong melos version during script execution Oct 29, 2024
Copy link
Collaborator

@spydon spydon left a comment

Choose a reason for hiding this comment

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

From a quick glance it looks like a sound idea!

/// i.e. usually the globally activated version. If there is one.
///
/// This can cause issues when melos is started with `dart run melos`,
/// which will prefer the melos version specified via (dev_)dependencies.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Melos should always run scripts etc with the version that is defined in dev_dependencies, even if it is started directly from the version on path.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, thanks for the quick review.

Is this a future enhancement request or already implemented? 🙂

Copy link
Author

Choose a reason for hiding this comment

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

The overarching goal of this PR is to eliminate ambiguity and surprises in the build process of Dart/Flutter applications. In line with that, I believe it would be ideal for Melos to behave in a consistent and predictable way.

Here’s what I think could be considered "unexpected" behavior:

  • If Melos uses different versions of itself for script execution (i.e. for subprocesses of itself)
    • This will be addressed with this PR!? :)
  • If Melos silently switches to a different version at startup (i.e., in the main process)
    • This could be especially challenging when intentionally testing a specific version.
    • Perhaps an optional strict mode (e.g., for CI environments) could help, where Melos would verify that its version matches the one specified in pubspec.lock.
      • The version in dev_dependencies might only define a minimum version, which wouldn’t fully address this, as you likely know as well. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a future enhancement request or already implemented? 🙂

It's already implemented, try pinning you melos version to an older version an see what happens. :)

The version in dev_dependencies might only define a minimum version, which wouldn’t fully address this, as you likely know as well. :)

If one wants a specific version of melos, why not just pin the version in pubspec? I don't see the need for any separate feature for this.

@bspinner
Copy link
Author

@spydon I'm unsure if this could/should/must be applied to other places as well, since this is the first time I had a look at the code.

@bspinner bspinner marked this pull request as ready for review October 29, 2024 14:28
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.

3 participants