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

Don't force uses-material-design: true for root pubspec #4486

Open
spydon opened this issue Jan 9, 2025 · 9 comments · May be fixed by flutter/flutter#160443
Open

Don't force uses-material-design: true for root pubspec #4486

spydon opened this issue Jan 9, 2025 · 9 comments · May be fixed by flutter/flutter#160443
Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@spydon
Copy link

spydon commented Jan 9, 2025

Environment

  • Dart version: 3.6.0

Problem

When you are using uses-material-design: true in any sub package with the new pub workspaces feature it complains that you also have to add this to the root pubspec.
A monorepo can have lots of packages that aren't bound to Flutter, it doesn't seem right that the root pubspec should need to be polluted with Flutter settings just because one of its sub packages uses it?

I'm sure there was a reason for this design decision, but is there no way around it?
And I'm also curious what the reason was?

Expected behavior

Flutter settings should be contained to their own pubspecs.

Actual behavior

You get the following error:

ERROR: package:example has `uses-material-design: true` set but the primary pubspec contains `uses-material-design: false`. If the application needs material icons, then `uses-material-design`  must be set to true.
spydon added a commit to flame-engine/flame that referenced this issue Jan 9, 2025
Currently we need to have a flutter section in the root pubspec.
Tracking it here: dart-lang/pub#4486
@sigurdm
Copy link
Contributor

sigurdm commented Jan 10, 2025

This was not a conscious decision - seems to be a bug in how flutter collects assets.

I was not easily able to reproduce when I tried. What sequence of commands are you running to reproduce this?

The message comes from https://github.com/flutter/flutter/blob/2dad95b21c15e41902f2ea0645d613246b364bce/packages/flutter_tools/lib/src/asset.dart#L392

@spydon
Copy link
Author

spydon commented Jan 10, 2025

I'm very happy to hear that this wasn't a conscious decision!

I thought it would reproduce whatever you did in the repo, but that was not the case apparently, what you have to do is run flutter test from the root of the repository, but pointing to the package that you want to run it in, so like this:

flutter test packages/a

If you run that in the MRE I've created here you'll get the error message:
https://github.com/spydon/mre_workspaces_material
The MRE is just a pub workspaces repository with one flutter package in it.

Maybe this isn't an issue at all then, it seems like it is mandatory to run flutter test from within the individual package root, which isn't that weird.

@sigurdm
Copy link
Contributor

sigurdm commented Jan 10, 2025

t seems like it is mandatory to run flutter test from within the individual package root, which isn't that weird.

I think so - though probably we could improve the messaging when that goes wrong.

@spydon
Copy link
Author

spydon commented Jan 10, 2025

Thanks for the quick replies, I'll close this and fix it on the melos side.

@spydon spydon closed this as completed Jan 10, 2025
spydon added a commit to invertase/melos that referenced this issue Jan 10, 2025
<!--
  Thanks for contributing!

Provide a description of your changes below and a general summary in the
title

Please look at the following checklist to ensure that your PR can be
accepted quickly:
-->

## Description

Due to dart-lang/pub#4486 examples must run
`exec` from their own directories (not sure why this wasn't the case
from the beginning).

## Type of Change

<!--- Put an `x` in all the boxes that apply: -->

- [ ] ✨ `feat` -- New feature (non-breaking change which adds
functionality)
- [x] 🛠️ `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
@spydon
Copy link
Author

spydon commented Jan 10, 2025

Aaah, now I found the real issue!
I updated the MRE.

So, if you have an example with uses-material-design: true and the package itself doesn't have that, which is very common when you want to present an example for a Flutter package, if you then run flutter test in the package itself it will complain about the example.

Reproduction instructions:

git clone https://github.com/spydon/mre_workspaces_material.git
cd mre_workspaces_material/packages/a
flutter test

Result:

package:example has `uses-material-design: true` set but the primary pubspec contains `uses-material-design: false`. If the application needs material icons, then `uses-material-design`  must be set to

@sigurdm Should this issue be filed towards Flutter instead?

@spydon spydon reopened this Jan 10, 2025
@spydon
Copy link
Author

spydon commented Jan 10, 2025

@sigurdm Should this issue be filed towards Flutter instead?

I guess it is correctly filed here, because if I remove the example from the workspace list and remove the resolution for it I don't get any warnings.

@sigurdm
Copy link
Contributor

sigurdm commented Jan 10, 2025

Well - it has to do with how flutter accounts for assets that doesn't work well with workspaces. So the fix has to happen in flutter/flutter.

But it is probably going to be me working on the fix as it is related to workspaces, so it doesn't matter hugely.

@sigurdm
Copy link
Contributor

sigurdm commented Jan 10, 2025

Trying your reproduction it seems that flutter/flutter#160443 will fix this issue.

@spydon
Copy link
Author

spydon commented Jan 10, 2025

Trying your reproduction it seems that flutter/flutter#160443 will fix this issue.

Great to hear that there is already a fix in the pipeline, thanks!

@sigurdm sigurdm added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants