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

Allow disabling of the thread checks as a "migration" - a pattern we can re-use #5425

Merged
merged 9 commits into from
Jan 21, 2025

Conversation

andydotxyz
Copy link
Member

Reasoning:

  • Build tags are great to run a specific build with a feature on or off but can be forgotten
  • App metadata is great for marking a project as migrated but are not part of the binary
  • So we have both. Use the tag if you want and add the migration to file when you're looking to make it permanent.

With this pattern we could (like the Go module tooling approach) change how apps work based on metadata, but build it in to the app through compiler flags.

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

@andydotxyz andydotxyz changed the base branch from master to develop January 17, 2025 14:52
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Thanks. I really like this approach. Added some comments inline.

Are we still adding cmd/fyne changes here or should we focus on the tools repo instead?

@@ -0,0 +1,5 @@
//go:build migrated_fynedo
Copy link
Member

Choose a reason for hiding this comment

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

Maybe migrated_threading would be better? Calling it FyneDo sounds a bit non descriptive to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd thing that "threading" is generic which is potentially problematic too.
The reasoning here is that the name matches an API that developers will have used to prepare this migration.

internal/build/migrated_fynedo.go Show resolved Hide resolved
internal/build/migrated_notfynedo.go Show resolved Hide resolved
internal/build/build.go Show resolved Hide resolved
internal/async/goroutine.go Outdated Show resolved Hide resolved
@@ -4,3 +4,6 @@
ID = "io.fyne.hello"
Version = "2.5.3"
Build = 2

[Migrations]
fyneDo = true
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as further down. Maybe calling this threading = true would make more sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

That assumes we will only ever have one threading migration. I'd rather be more specific

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, true but I'm not all too enthusiastic about the previous name either. I would like to hear what @dweymouth thinks.

Copy link
Member

Choose a reason for hiding this comment

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

If we are to do a v3.0.0, then we can remove the tag and use the same tag again in the future in case we want to do one more thread migration (which I kind of think is unlikely before v3.0.0). I am still in favour of migrating threads as a better name (fynedo sounds a bit strange) but I approved this for now so it can land and be useful for more people.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interestingly I think that re-use as you described would be a very bad thing. A migration system where what it means changes over time seems like it will be confusing and probably problematic.
Hence I'm happy we are sticking with what is here as it's explicit.

@andydotxyz
Copy link
Member Author

Are we still adding cmd/fyne changes here or should we focus on the tools repo instead?

Good question. I wanted to do it here as one PR for discussion, will migrate to tools once agreed.

@Jacalz
Copy link
Member

Jacalz commented Jan 17, 2025

FYI: With the last commit, the thread check lookup being variables means that stuff aren't optimized away any more. You could just revert that commit and use a sync.One to store that metadata value globally once instead.

@andydotxyz
Copy link
Member Author

@Jacalz i think it's good now?

@Jacalz
Copy link
Member

Jacalz commented Jan 19, 2025

There is the naming discussion that I pinged Drew about still.

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Let's just land this in the meantime. We can change the name in the future depending on what @dweymouth thinks about it.

@andydotxyz andydotxyz merged commit d94129d into fyne-io:develop Jan 21, 2025
12 checks passed
@andydotxyz andydotxyz deleted the fix/smootherfynedotransition branch January 21, 2025 11:50
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.

2 participants