search

ffmpeginteropx / FFmpegInteropX

FFmpeg decoding library for Windows 10 UWP and WinUI 3 Apps
Apache License 2.0
211 stars 53 forks source link

Centralize Windows SDK setting into Directory.Build.props #432

Closed JunielKatarn closed 2 months ago

JunielKatarn commented 2 months ago

Description

Sets the WindowsTargetPlatformVersion MSBuild property to 10.0.22621.0.

Motivation

Current/latest Visual Studio release uses Windows SDK 10.0.22621.0 as its default/mandatory for UWP workflows (it can't be uninstalled using the VS installer).

Setting all projects to use this version as default should save developers a couple GBs from their installation base.

The "default" SDK version changes every few years so updates of this type would be very sporadic.

Details

Testing

MediaPlayerCPP app works normally.

softworkz commented 2 months ago

I think that this PR should not do two things at once:

If you could remove the version changes, so that it produces the same result like before (and get rid of all those whitespace changes), then I'm all for it.

(Since there's the .,local.props mechanism now, it will be easy to have different SDK versions locally)

brabebhin commented 2 months ago

I think the 2 can go together, it will be pretty obvious if the build will not work due to property mismatches.

softworkz commented 2 months ago

I think the 2 can go together, it will be pretty obvious if the build will not work due to property mismatches.

Example: If the version change is a separate commit, you can easily revert to the previous SDK by doing a revert commit (locally or in a fork). If it's in a single commit, you can only revert the whole change and this might no longer work once other conflicrting changes are made ion top.

It's a good practice to separate such things, that's all I'm saying.

brabebhin commented 2 months ago

This is fine here, we mostly do sdk updates as side changes.

lukasf commented 2 months ago

I am with @softworkz on his point of separating things. This PR does three totally unrelated things: 1. Move SDK version to central file, 2. Update SDK version, 3. Totally unrelated whitespace changes which make PR bad to review.

I am all-in on number 1. I would immediately approve if it was only this.

For number 3, if you want to apply auto formatting, it's a lot better to do this in a separate PR - or at least in a separate changeset. Mixing whitespace changes with functionality changes in a single commit is really bad for reviewing, because you can easily miss important changes in the clutter of whitespace changes.

For number 2: Actually, I am not really a fan of upping the Target SDK version without real need. While it is true that the Min verion does not change, it still forces you to upgrade the Traget SDK of every single project where the lib is used! You cannot use a lib whose Target SDK is higher than your own Target SDK. I don't like it when I am forced to upgrade my target versions, just because a lib decided to do so (and then even without making use of a single new feature of that SDK). This is the reason why I have been very conservative with updating the SDK version. I only done it on rare occasions - or when I really wanted to use a new feature (yes there were these good old times, where Microsoft actually added new interesting features to the SDK).

Same for the .net Version. Why upgrade to .net 8 and force every user of the lib to upgrade as well, when everything works just as well when the lib is built on .net 6?

Disk space is a pretty poor reason, when the whole build environment with IDE, repos, intermediate files etc etc easily goes into the area of 100GB and more. 2GB more or less don't make any difference IMO. When moving the versions into the central file, it is easy for you to override the version, if you really need to save those 2 GB.

Maybe I will think about this again, but right now I am not convinced that we should upgrade the SDK version.

softworkz commented 2 months ago

For number 3, if you want to apply auto formatting, it's a lot better to do this in a separate PR - or at least in a separate changeset. Mixing whitespace changes with functionality changes in a single commit is really bad for reviewing, because you can easily miss important changes in the clutter of whitespace changes.

He can't separate the whitespace changes because he is working in an IDE which applies .editorconfig rules to the whole file on saving (VS works very differently). But we don't need those rules at all. My PR https://github.com/ffmpeginteropx/FFmpegInteropX/pull/433 removes these.

softworkz commented 2 months ago

I just realized that there's a much bigger problem with this PR - it's not directly @JunielKatarn 's fault. This PR ist just closing a circle where things become apparently short-circuiting.

That's a big problem, because it's really confusing.

As a side note: With the approach that I had suggested (using a common.props that is explicitly imported), it would be different. In that case, the SDK version properties in the common.props file would take precedence.

But that wouldn't make this specific situation much better. I think we shouldn't have both: a common props file and defaults in the build scripts.

lukasf commented 2 months ago

I just realized that there's a much bigger problem with this PR - it's not directly @JunielKatarn 's fault. This PR ist just closing a circle where things become apparently short-circuiting.

Yeah I also thought about that. But I think we can solve this in an elegant way, keeping both the build script override and the local props override. Usually, in the project files these properties are defined like this:

<WindowsTargetPlatformVersion Condition=" '$(WindowsTargetPlatformVersion)' == '' ">10.0.22000.0</WindowsTargetPlatformVersion>

What our build script does is setting this property to the specified value, before starting the build. Then the property is not empty, and it is not overriden by the project file prop. If we use the same definition approach in our Directory props file, we can define the default SDK version there, while still allowing to override it through either the local props file or the build script. In the build script, we could default the SDK version to an empty string. Then normally, the default of the props file would be taken - unless we explicitly specify an SDK version in the build script. Then we only have one place where our default versions are specified, but we have two different ways to override it.

An alternative would be removing the sdk version from the build script. But I find it quite handy that we can easily produce different builds for testing out stuff, without changing a single file.

softworkz commented 2 months ago

I think your first idea makes the most sense. It's fine to be able to override the SDK version via a script parameter. The only case that needs change is when invoking the script without SDK version parameter, in which case the value in Directory.Build.props should be used.. Means, the parameter in the PS script should not .have a default value. The one thing to figure out is how to handle that "empty sdk version string" when invoking the build using the /p: parameter..

JunielKatarn commented 2 months ago

@lukasf

To be honest, drive space was the only reason I meant to increase the SDK version. If you deem it insufficient, I have no problem reverting to the previous version. It seems we all agree on the value of centralizing the version setting in Directory.Build.props.

Regarding the spaces issue, I think @softworkz already noticed it was not an intentional change. But I strongly believe it is a correct change. I'll open a separate discussion for this, but I think it would be a reasonable requirement for this project's contributions to require EditorConfig rules compliance.

Clearly the .editorconfig file itself was edited without abiding by its own rules (can easily happen if using, say, Notepad).

If it's all that important, I can re-insert the unnecessary spaces in Notepad to make this PR fully compartmentalized and then apply the whitespace fix in a follow-up one. That said, this type of issue will arise again when another file with non-complying format gets edited.

The alternative is to just delete the .editorconfig but I would strongly vote against that; this feature solves more problems than it introduces as long as it's set up correctly ("correctly" = its rules reflect what is actually wanted).

I'll update the PR on Monday.

softworkz commented 2 months ago

Regarding the spaces issue, I think @softworkz already noticed it was not an intentional change. But I strongly believe it is a correct change. I'll open a separate discussion for this, but I think it would be a reasonable requirement for this project's contributions to require EditorConfig rules compliance.

Requiring rules compliance is valid, Many projects have such rules, but this is usually scoped to changed and added lines, but not to unrelated lines. Besides that, code style changes are usually required to be separate (in such project), and there are good reasons for doing so. You can't imagine how many times I've been glad about that separation when skimming trhough version control history and an almost equal number of times I've been frustrated when encountering commits where it wasn't done.

The trailing whitespace was introduced here: https://github.com/ffmpeginteropx/FFmpegInteropX/commit/8b88ad5e78e65c007000ff20e4e22c5dbe14174e

@brabebhin - Did you use any tooling to create these entries? Because when there's some tooling which adds that whitespace, then it would be contradictive to have an editorconfig rule to trim it (then it would go ping-pong).

But otherwise, a separate PR (or commit) for the white-space changes would be perfect!

brabebhin commented 2 months ago

The tools I use are Visual Studio and Git Extensions.

I don't remember how that came to be.

softworkz commented 2 months ago

Then it was probably VS and it's better to remove that rule for trailing whitespace removed. BTW, the editorconfig spec itself doesn't have any rules about trailing whitespace, which means that the file is valid as is.

softworkz commented 2 months ago

That said, this type of issue will arise again when another file with non-complying format gets edited.

Not when edited with Visual Studio, which applies editorconfig rules only to changed lines.

The alternative is to just delete the .editorconfig

The vast majority of editorconfig rules are not of the kind that the editor applies them automatically (VS only to edited lines, VSCode seemingly to all lines). All other rules are rather just about indicating things which do not conform to the defined rules, while it's up to developer to change them. The PR that I've submitted removes only those few rules which cause the editor to make changes on its own. There's no need to remove all the other rules that @brabebhin has added (like naming rules)..

softworkz commented 2 months ago

If it's all that important, I can re-insert the unnecessary spaces in Notepad to make this PR fully compartmentalized and then apply the whitespace fix in a follow-up one.

I feel you. Really. I've been there. So many times that I made contributions to projects where I was asked to make changes from which I thought they wouldn't be necessary. I had to rewrite history, modify, split and merge the commits I had, often several times until everybody was satisfied. Often I thought to just bail out and cancel my contributions. Few times I even did, but most of the time it was part of my paid work, so I just had to do it. I used Git interactive rebase from the command line where a text editor opens and you can change the content to re-order or squash etc, and I hated doing that like nothing else. I came to a point where I realized that I need to turn my "wtf" into a "sure, no problem" mentality. I had a number of ideas for how a really good Git tooling should be implemented and after looking at more than a dozen of Git applications, I came across the one that I'm using ever since, I had recently shown it in a video. Splitting your commit into two commits (one for white space changes and one for functional changes) can be done in less than 60s. Since then, I've never been annoyed again by any such requests, and it's always a "sure no problem" kind of thought. I can only recommend to re.think your Git tooling. Re-adding the spaces with notepad is a really horrible way to split that commit. Would annoy me massively ;-)

lukasf commented 2 months ago

@softworkz Which git client do you use? The rework history stuff looked pretty advanced in that video you posted.

softworkz commented 2 months ago

It's SmartGit from Syntevo (German company) and it's cross-platform (Win, Linux, Mac). Change the default window to "Log Window" (their "Standard Window" is a new addition, meant to be more simple, but it doesn't have that great logic of the log window. Standard Window is more like other tools have).

I've seen many Git UI clients which often look similar at first glance but none comes even close. This has totally changed the way I work with Git. It does not just allow to do everything with ease that you would expect, but many more things beyond. There's a bit of a learning curve, but it's fun to discover over time how well thought out it is.

Left side bar top is a list of your local repos. The center panels shows the list of commits and the right panels show commit info and list of changed files (either of a commit or current state. At the bottom are two panels showing the changes (switch off "Compact" mode). You seamlessly go through all changes of a commit, it automatically jumps to the next file when no more change in the current. This makes it so much easier to review changes like for example with Visual Studio. It's all in the same window.

What*s unique to the log window is the way to work with branches. In the side panel left, you have a tree view which shows local branches, and all remotes with all branches (plus tags, refs and PRs for GitHub repos). Each branch has a checkbox to indicate whether you want to see the commits in the commit log in the center. This allows you to compare everything to everything without clutter and without restrictions. Select any two commits in the commit list and you get the difference as if it was a single commit's changes.

You can view the changes for a specific file for which you get a similar window - again with checkboxes, so you can compare commits and contents of that file across all selected branched.

You can drag/drop commits to re-order, split, commits, modify commits or squash commits. There's an integrated editor for conflict resolution and staging/unstaging individual lines or blocks of changes. Splitting a commit like in this PR to separate whitespace from functional changes is a matter of 60 seconds (right-click Modify, unstage file, open editor, stage functional line changes, save, click Commit. Stage file (with the remaining whitespace changes), enter commit message "Trimming Whitespace", click Commit. Click Push, confirm Force-Push. Done.

softworkz commented 2 months ago

What I didn't know before is that all the operations you do are always directly reflected in Git itself. That means that there's no discrepancy between tooling and you can use it in parallel to Visual Studio's Git integration. Things are always in sync. When you start a rebase operation in VS, you see that reflected in SmartGit and you can do conflict resolution there and for example confirm/abort from the command line. You can do everything everywhere at any time and both, VS and SmartGit are updating their UI accordingly. That's really cool, and so you don't need to make any choice for one or the other.

brabebhin commented 2 months ago

@softworkz the tool integration thing is similar to how git extensions works as well (which is also a pretty cool tool) and it is probably because all of these things are wrappers over git.exe. Visual Studio only has nice diff and merge UI, otherwise its own git implementation kinda sucks and lacks important stuff like submodules and stashes.

softworkz commented 2 months ago

it is probably because all of these things are wrappers over git.exe.

Yes exactly.

Visual Studio only has nice diff and merge UI, otherwise its own git implementation kinda sucks

I see it the same way: The diff is great (and exists for quite some time), but all the recent implementations are just done in a very bad ways. Not well thought out at all.

and lacks important stuff like submodules and stashes.

There's submodule support available, but it's behind a feature flag. You need to install the extension for toggling feature flags to enable it.

lukasf commented 2 months ago

It's SmartGit from Syntevo (German company) and it's cross-platform (Win, Linux, Mac).

Sounds good. The price tag does not make it an insta-buy though. How is the performance? Does it have kind of a cache to speed up things? Actually, I feel quite comfortable with VS git. While it does not have those advanced features you describe, it's at least a lot better than most other free git clients. And it has line staging, which is important for me. But the performance is really annoying, on all the git tools I tried. This is the biggest pain point for me. Each time I select a different branch (even if I had it selected a minute before) it takes an aweful long time to populate the history. Enabling commit graph in VS did not really help.

softworkz commented 2 months ago

Sounds good. The price tag does not make it an insta-buy though.

They used to have a free license for open source projects - or more precisely even any "non-commercial use". I think it's no longer just a checkbox on first start (like it's been for a long time), but you can request it in some way.

How is the performance?

Excellent in general. Also, you can mark repos as "favorite". These will be synced in background automatically, so there's no wait. It has its own rename detection which can be slow when a repo has thousands of files, but it can be switched off.

I feel quite comfortable with VS git.

I do use both in parallel. Whichever gives me better or faster results depending on the case. An advantage of using diff/merge of VS is that you can get Intellisense (+ editorconfig + ReSharper + StyleCop) warnings and errors while merging.

And it has line staging, which is important for me.

I find this horrible in VS. The way that they are drawing those overlays is annoying and distracting and staging is slow. Also there is no unstaging. In SmartGit, there's a gutter in the middle of the diff view with arrows (left to right and right to left) to stage or revert and you can view another diff of the staged file where you can unstage in a similar way.

While it does not have those advanced features you describe,

Since using it, those are no longer "advanced features" to me but core features that I'm using always to properly shape my commits before pushing to any remote. I even do this for projects where I'm to only committer. I can give tons of examples for why I'm doing so and what benefits it provides.

The most typical case is: Doing part of an implementation as commit A. Then I implement another part as commit B. Then I realize that a change is needed which logically belongs into commit A. I commit it as A1. Re-order so that it comes right after commit A and then I squash it into commit A. If I wouldn't do that:

I could continue this even further... All these things happened to me or I've seen them happening, these aren't any constructed theoretical cases. I always knew that this could be avoided by having properly shaped commit sets or PRs, but the effort to achieve that was just insanely high before I found out how to do it with ease.

Each time I select a different branch (even if I had it selected a minute before) it takes an aweful long time to populate the history.

As mentioned, you can show/hide commits of individual branches via those checkboxes. It takes less than 100ms for the list to update (no matter if branch is local or remote). Checking out a different branch takes less than a second (except extreme cases). If VS has a project open from that repo, it might still take a while to reload the projects, but the branch switch is done in less than a second.

JunielKatarn commented 2 months ago

@lukasf Reverted the version bump as requested. This indirectly discarded the unintended space trimming.

Ready to merge.

... never mind the branch name.