ffmpeginteropx / FFmpegInteropX

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

Allow setting repo-global MSBuild properties in config file #431

Closed JunielKatarn closed 2 weeks ago

JunielKatarn commented 2 weeks ago

Description

Enables global, centralized MSBuild property setting and overriding.

Motivation

Currently, build behavior needs to be defined or updated in several project files (i.e. Windows SDK version) causing potentially inconsistent settings and more complex pull requests.

Details

softworkz commented 2 weeks ago

I very much like the idea of having common build properties in multi-project solutions and I virtually always do that.

Personally, I'm not a fan of using Directory.Build.props (and .targets) for this purpose though. For the following reasons:

What I usually do is to add a common.props file to the root and then import it explicitly only those projects which are supposed to use them like so:

image

.

I like the idea of allowing to have a local file to override properties (which is git-ignored). In a common.props file, it could be included like this:

image

This way, there's no need to "invent" a different file extension (or actually mis-use the .user extension) and you get proper IntelliSense in this file.

softworkz commented 2 weeks ago

More clearly:

lukasf commented 2 weeks ago

It is a good idea to move common things into centralized props files, to prevent repetitive declarations in multiple project files. It's really annoying having to update e.g. version numbers throughout multiple projects. I don't think that using a directory props file is an issue for our repo. It's not so huge anyways, and we could easily put an empty props file into the lib folder, to prevent side effects in any of the sub repos.

I agree with @softworkz that a local/user override file should have the .props extension as well, to get proper intellisense support.

JunielKatarn commented 2 weeks ago
  • Use of different file extension: would be an objection from my side

I have no preference at all regarding the extension. I chose that one because it's already in .gitignore and made sense to me. I can make sure the non-versioned file ends with .props.

  • Approach using common.props: is just a suggestion

In my experience, this is not either-or. I have worked on projects on which both exist in a complementing way.

What you point out about Directory.Build.props is true, but it is not a problem. The file is not evaluated "too early", but "the earliest" which can be the intended behavior for some properties. It being imported implicitly is also not a problem, so long as the properties set on it are clearly something we want as a global default. In exceptional situations, such properties could still be overridden per project file. Such behaviors are by design and not by omission.

If needed, a common.props that's imported explicitly can be added later, when/if needed.

JunielKatarn commented 2 weeks ago

@lukasf ready for merge.

softworkz commented 2 weeks ago
  • Approach using common.props: is just a suggestion

In my experience, this is not either-or. I have worked on projects on which both exist in a complementing way.

True, the Dir.B.props mechanism is useful. I tend to use it rather as a band-aid, to quickly achieve things without needing to edit all project files. The larger a solution gets, the more troublesome it becomes to manage builds with this approach, but I agree that it doesn't matter much here.

This PR has 5 commits with superceded/obsolete changes, can you please remove these?

JunielKatarn commented 2 weeks ago

Coiuld you please cleanup the commits?

Can you clarify? The changes to be merged are minimal and only the intended ones.

Do you mean the intermediate commits that changed all line endings in .gitignore and .editorconfig because this PR was edited in a UNIX machine?

They won't exist in the target branch once the PR gets squash-merged.

softworkz commented 2 weeks ago

Can you clarify? The changes to be merged are minimal and only the intended ones.

Do you mean the intermediate commits that changed all line endings in .gitignore and .editorconfig because this PR was edited in a UNIX machine?

This PR has 5 commits and 4 of them have changes to .editorconfig. The 5th one changes line endings in .gitignore. None of these things should be there. Please remove all this, squash locally and force-push your branch.

JunielKatarn commented 2 weeks ago

Can you clarify? The changes to be merged are minimal and only the intended ones. Do you mean the intermediate commits that changed all line endings in .gitignore and .editorconfig because this PR was edited in a UNIX machine?

This PR has 5 commits and 4 of them have changes to .editorconfig. The 5th one changes line endings in .gitignore. None of these things should be there. Please remove all this, squash locally and force-push your branch.

I don't think you read my other comment. Those changes happened because of the EOL behavior on UNIX machines. I had to modify EditorConfig so the involved files do not get polluted (consequence of having inconsistent checked-in content vs EditorConfig rules).

I am aware everything can be squashed and force-pushed. There is no need for that if we squash-merge (the intermediate commits won't be part of winui-build).

Please help me understand why you are demanding this, which corrects nothing and adds more work on my side.

softworkz commented 2 weeks ago

I don't think you read my other comment. Those changes happened because of the EOL behavior on UNIX machines. I had to modify EditorConfig so the involved files do not get polluted (consequence of having inconsistent checked-in content vs EditorConfig rules).

I don't care how you are working, what matters is the result you are delivering and that's not right.

There is no need for that if we squash-merge (the intermediate commits won't be part of winui-build).

We don't squash-merge. Someone hits the "Merge Pull Request" button on GitHub and this doesn't squash.

JunielKatarn commented 2 weeks ago

I don't care how you are working, what matters is the result you are delivering and that's not right.

Now you are being stubborn and hostile. I don't think you should be gatekeeping code reviews without supervision.

There is no need for that if we squash-merge (the intermediate commits won't be part of winui-build).

We don't squash-merge. Someone hits the "Merge Pull Request" button on GitHub and this doesn't squash.

If this is a rule that has been openly and clearly discussed somewhere else (not just by you), I will understand and respect it.

If not, either you are not very familiar with GitHub or are intentionally leaving out the fact that the Merge button has a "Squash" option.

JunielKatarn commented 2 weeks ago

@lukasf @brabebhin Unfortunately, some personal-aimed animosity has developed and I don't think progress can be made this way. I would prefer to wrap up this particular PR's discussion with you.

If you think this PR's contents are correct and non-disruptive, would you please approve?

I do have a question about the merging convention. Is it acceptable for you to squash-merge (see drop-down arrow in the Merge pull request button) so don't have to edit the commit history locally?

Thanks.

softworkz commented 2 weeks ago

If not, either you are not very familiar with GitHub or are intentionally leaving out the fact that the Merge button has a "Squash" option.

Apologies - I haven't used that for quite some time. But still that doesn't matter because it makes no sense to merge something without being sure what will be the exact result beforehand (if it would be squashed).

I don't think you should be gatekeeping code reviews without supervision.

I'm just telling your the same that @lukasf would be telling you tomorrow.

Now you are being stubborn and hostile.

I'm just annoyed having to discuss a PR which could be re-created correctly within 3 minutes.

JunielKatarn commented 2 weeks ago

After reading this I see I may have misinterpreted your conviction as personal aversion. I will scale my tone down.

Apologies - I haven't used that for quite some time. But still that doesn't matter because it makes no sense to merge something without being sure what will be the exact result beforehand (if it would be squashed).

I work with GitHub's squash merge on virtually a weekly basis (in some projects we even make it default/mandatory).

Squash merge does two things:

  1. Squashes all commits in the source branch into one.
  2. Merges that commit into the target branch with the exact contents you would see in the files section. I can guarantee this and have been doing it for years.

I'm just annoyed having to discuss a PR which could be re-created correctly within 3 minutes.

And I hope you believe me I am not just being lazy precisely for that reason, but the tools to turn those 3 minutes into less than a second are right there, available to all approvers. See this sample:

image

This actually opens the window for a useful discussion: Should regular merge be used? Could we all benefit from switching to squash-merge as the only option for PRs?

I personally don't know any benefits of keeping the source branches' exact commits when doing PRs for this type of project, but I'm happy to abide by any convention established by everyone.

I'll wait for Lukas's decision on this.

softworkz commented 2 weeks ago

After reading this I see I may have misinterpreted your conviction as personal aversion. I will scale my tone down.

All fine, I didn't mean to be hostile, just productive :-) My actual intention was to approve your PR after getting it clean - not the opposite. If you squash the commits locally and remove the .editorconfig changes, I'll approve it instantly. (I don't do merges though. I have permission, but that's not my role here).

I work with GitHub's squash merge on virtually a weekly basis (in some projects we even make it default/mandatory).

Squash merge does two things:

  1. Squashes all commits in the source branch into one.
  2. Merges that commit into the target branch with the exact contents you would see in the files section. I can guarantee this and have been doing it for years.

My concern are the EOL changes, as to whether those will get equalized-out against each other. The problem is that there's no preview, because it's a 2-step procedure and merging into a production branch is a critical operation. In those branches you don't want to force-push to fix-up things later. And when you fix-up an EOL change with a subsequent commit, this will create trouble until eternity when rebasing and commits are being re-played which have eol changes.

We don't squash-merge. Someone hits the "Merge Pull Request" button on GitHub and this doesn't squash.

If this is a rule that has been openly and clearly discussed somewhere else (not just by you), I will understand and respect it.

By saying "we don't squash-merge" I simply meant to say that I have never seen this being done here.

This actually opens the window for a useful discussion: Should regular merge be used? Could we all benefit from switching to squash-merge as the only option for PRs?

I personally don't know any benefits of keeping the source branches' exact commits when doing PRs for this type of project, but I'm happy to abide by any convention established by everyone.

I could spit out quite a list of reasons, but let's save that discussion for another time. :-)

brabebhin commented 2 weeks ago

We do have squash merge, it is the default merge action, we use it most of the time, as our PRs usually have a lot of back and forth commits with trying things out, and it makes no sense to have everything in.

I don't think any of gits weird features like history edit or force pushes are generally needed on a day to day basis. It doesn't matter if one commit has something bad in it if a future commit fixes it, i very much like linear history.

I will sort this out in the evening. I want to play with this PR a little bit anyways.

While we do have high standards for quality here we also like to keep it relaxed and friendly, so I very much urge everyone to just stay friendly :) open source contributions with strangers on the internet requires a different approach to communication than with the folks in the office.

softworkz commented 2 weeks ago

It doesn't matter if one commit has something bad in it if a future commit fixes it, i very much like linear history.

This is correct in all cases except when it's about EOL changes.

We do have squash merge, it is the default merge action

There's no way to set a default merge action, it just shows you method you've used the last time.

None of my PRs has been squash-merged, also none of those PRs from @lukasf which I had observed were squash-merged, But I see now that you did that at times.

@JunielKatarn - I apologize for my incorrect statement regarding squash-merges. I've followed quite a number of PRs over time and in none of those, it was done. There have also been discussions about how to combine or separate individual commits in a PR (which would have been pointless when squash-merging). These two things shaped my perception that squash-merges wouldn't be done.

Yet it's not really a question of squash-merge (yes/no). What I said still stands for me: I find it risky to rely on that squash-merge to do what it's supposd to do (especially due to eol changes being involved) and I think that having a clean set of commits in a PR is not asked too much. But that's just my view of this.

brabebhin commented 2 weeks ago

We usually have mixed collaboration on PRs, squashing locally or doing any sort of forced push is a bit of a noisance. The kind of one man PR is usually rare here. We never had problems with squash merge, the whole cppwinrt migration is an example of huge PR squashed no problem.

softworkz commented 2 weeks ago

We usually have mixed collaboration on PRs, squashing locally or doing any sort of forced push is a bit of a noisance. The kind of one man PR is usually rare here.

The last 10 PRs which were merged by @lukasf were not squash-merged. Only the ones that you had merged.

squashing locally or doing any sort of forced push is a bit of a noisance

Ah, I think I got an idea now for where the mismatch comes from.

When I am talking about squashing, history rewriting and force-pushing it's something like this:

https://github.com/user-attachments/assets/7f5f45d6-aced-4b09-bab4-608cccb8b6f0

When you say that you consider it a nuiisance, I wonder how you are doing these things? Git command line? In that case I'd uinderstand - having to do it that way would freak me out like... :-)

brabebhin commented 2 weeks ago

I avoid git commandline like the plague. It cannot be used in any sort of mid (like this one) to large projects imo but all my admiration for those who can use it. It is a good torture tool for students though.

I wasn't aware lukasf does merge commits, I'm pretty sure he was the one who suggested squashing, but it is probably a case by case choice which is also fine. Anyways, I think we are approaching the problem the wrong way.

Note that the video does show a force push. I don't think doing a history rewrite at this point is necessary, might be in the future. I have now understood why @JunielKatarn is having problems with line endings, let us take a step back and think it through, do some testing. Remember we are not under any pressure to merge PRs ASAP unless it is some world ending bug that causes epilepsy attacks or crashes people's eardrums.

softworkz commented 2 weeks ago

I avoid git commandline like the plague. It cannot be used in any sort of mid (like this one) to large projects imo but all my admiration for those who can use it. It is a good torture tool for students though.

LOL, absolutely!

Note that the video does show a force push. I don't think doing a history rewrite at this point is necessary

Yes. This is something that you do on a PR branch when the PR is pending and you are asked to do some changes.

I have never worked on or even seen a repo with a professional context - no matter whether (renowned) open-source or commercial - who are doing squash-merges - and there are plenty of reasons for avoiding it, but that's another (long) story. Let's focus just on this part:

Like shown in the video - that's a matter of seconds.

The Second Part

The .editorconfig changes need to be removed. These are changes which had been included already in the previous PR https://github.com/ffmpeginteropx/FFmpegInteropX/pull/430 - and we've just rejected these changes. This is what had upset me a bit yesterday. But after all - those changes do not belong into this PR anyway.

@JunielKatarn - How about this: submit the .editorconfig change in a separate PR for discussion. I think it would help a lot when you could precisely explain the workflow (step-by-step and the specific tools you're using) for which you think those changes would be required.

JunielKatarn commented 2 weeks ago

The .editorconfig changes need to be removed. These are changes which had been included already in the previous PR #430 - and we've just rejected these changes. This is what had upset me a bit yesterday. But after all - those changes do not belong into this PR anyway.

@JunielKatarn - How about this: submit the .editorconfig change in a separate PR for discussion. I think it would help a lot when you could precisely explain the workflow (step-by-step and the specific tools you're using) for which you think those changes would be required.

I am now at a Windows machine and was able to remove the changes without running into the EOL issues. I did not test autocrlf=input as @brabebhin suggested, but I will set up an experimental branch for further discussion.

Please note, the changes I had added to EditorConfig are not the core of what was rejected in #430. The now reverted changes in this PR would have not induced the merge conflicts you warned about because:

  1. Both files are already LF-ended in the repository.
  2. Windows Git would have always checked them in as LF due to autoCRLF.
  3. Unix would now check them in as LF. Without those changes (again, now reverted), non-Windows Git will check those in as CRLF unless some local config changes are made (which I think shouldn't be needed).
JunielKatarn commented 2 weeks ago

I wasn't aware lukasf does merge commits, I'm pretty sure he was the one who suggested squashing, but it is probably a case by case choice which is also fine. Anyways, I think we are approaching the problem the wrong way.

Perhaps he had the intention of doing squash merges but that option was not set as the default. This is all configurable. I'll wait for him to see what's the intended/effective state of things.

I'd personally prefer to have GitHub's merge deal with squashing. Most projects I work on, without my input, do this and saves everybody a good amount of time.

I have now understood why @JunielKatarn is having problems with line endings

I am aware defaults (use Windows only with autoCRLF on) will work for most people, but my previous PR aimed to make the repo EOL issue-free (with a compromise). I'll set up an experimental repo and start a separate discussion.

My current changes to EditorConfig, while completely innocuous to Windows users, has have been reverted.

brabebhin commented 2 weeks ago

@softworkz force push is ok if you are the only one working on a branch. If there's multiple people, it gets messy very fast. In a normal environment it is one human per PR. Here we usually have several people on the same PR.

JunielKatarn commented 2 weeks ago

I'm not sure about those whitespace changes, and whether the remaining .editorconfig changes should be in this PR, but for me it's okay now.

Ah, didn't even think of them.

There IS one modification to that file that I wouldn't defend: The props extension was configured twice. I removed one of its instances for obvious reasons.

The rest of it was EditorConfig doing its job: Applying its rules to the edited file.

While not an intended change, I think it should stick. Even if I reverted it now it would show up again in the future if someone uses an EditorConfig-enabled IDE (very likely, by the way).

softworkz commented 2 weeks ago

@softworkz force push is ok if you are the only one working on a branch. If there's multiple people, it gets messy very fast.

Not just messy - this is not a viable pattern at all then.

Here we usually have several people on the same PR.

A PR is normally not a means of collaboration, You would work on a specific branch and do PRs (or pushes) to that branch. When the work branch is ready to be merged, then one person would have to do the cleanup of all commits from the work branch into a new branch from which the PR would be created to merge it into master. That's how I'd do it, but it's not the only way of course.

softworkz commented 2 weeks ago

The rest of it was EditorConfig doing its job: Applying its rules to the edited file.

While not an intended change, I think it should stick. Even if I reverted it now it would show up again in the future if someone uses an EditorConfig-enabled IDE (very likely, by the way).

The question might be which IDE it is: Which one did you use actually?

I don't mind the change itself, but I had the same thought like you: If it gets merged now, would it be reverted later when edited in Visual Studio. As I assume that it wasn't VS which made those whitespace changes?

softworkz commented 2 weeks ago

Talking about IDEs - I re-read some of your comments and I realized that you described some behavior which doesn't match the behavior of Visual Studio.

For example, you said that when .editorconfig would specify a line ending of LF for a specific file type, then all line endings would be changed to LF when the file is saved, right? (or same if it was CRLF )

brabebhin commented 2 weeks ago

@softworkz it is difficult to do collaboration on a branch with such heavily async model that we are using. Imo the way we work is generally fine for the scope of the library.

Trying to sync 3-4 people across different time zones is very difficult. So we stick to PR as collaborative tools.

For example I'm now at work and have no way of contributing to this PR by code. Well be for the next 8 hours too.

softworkz commented 2 weeks ago

@softworkz it is difficult to do collaboration on a branch with such heavily async model that we are using. Imo the way we work is generally fine for the scope of the library.

Trying to sync 3-4 people across different time zones is very difficult. So we stick to PR as collaborative tools.

Actually that's not that much different at all. It's always a branch, no matter whether there's a pending PR for it or not. At the end, you could still close that PR, one person does the cleanup of commits and creates a new PR for actually merging it.

softworkz commented 2 weeks ago

I am aware defaults (use Windows only with autoCRLF on) will work for most people, but my previous PR aimed to make the repo EOL issue-free (with a compromise). I'll set up an experimental repo and start a separate discussion.

My current changes to EditorConfig, while completely innocuous to Windows users, has have been reverted.

The problem here is that youi think that I wouldn't understand your intentions and the way how you are thinking that it would work. But in fact I do not only understand exactly what your idea is, I'm rather a few steps beyond, and that's why you didn't understand what I was stating.

Granted, I really had a hard time explaining things accordingly, because I didn't know what your mission is. Allow me to say that it really wasn't helpful that you tried to keep us in the dark with regards to what you are actually doing.

But I finally got a picture:

Please be honest this time: Right or wrong?

(I mean roughly - without nitpicking)

PS: I owe you an explanation for why I said I'm steps ahead in understanding and why your statement

My current changes to EditorConfig, while completely innocuous to Windows users

is not right. I know why you think that - and that thinking surely makes sense - but it's everything but innocuous instead. I have tried to explain it before, but I wasn't able to convey it properly. Maybe it's better to do a video showing it practically.

What I essentially wanna say: The better we know what you are trying to do, the better will we be able to help

softworkz commented 2 weeks ago

The better we know what you are trying to do, the better will we be able to help

I really mean it like that. Here are some ideas to explore:

1. Remove end_of_line statements

Above I said that you have claimed that the default eol style for .editorconfig files would be CRLF and that the spec doesn't have such defaults. Could it be that you rather meant that the global default eol style [*] is set like that in our editorconfig?`

In that case, the good news is this: None of those end_of_line definitions needs to be there. Visual Studio knows and infers and deals with that perfectly well without any of those entries.

2. Try unset

If the above (removal) doesn't work out, we could try to explicitly unset the eol style like:

end_of_line = unset

3. Local Ignore

Another technique you could try is to locally ignore the .editorconfig file. IIRC it is possible to have git ignore a certain file just locally. That means that would be able to make whatever changes you like to your local .editorconfig file.

4. Radical Approach

Remove .editorconfig altogether. There's absolutely no need to have that file with regards to Visual Studio operation. It's not built upon this. More reasonable of course: comment out everything and then find out whether or wich declarations are really needed for something relevant.

softworkz commented 2 weeks ago

@JunielKatarn - isn't this commit from yours actually? https://github.com/ffmpeginteropx/FFmpegInteropX/commit/cca2e239a897d8780ced65b82ce27d95d0fa458c

This commit is from 2 years ago and is originally adding the .editorconfig file to the project. Now I see that this had already been an attempt to force the line endings in FFmpegInteropX to LF.

Two weeks latrer, @lukasf has changed it to CRLF. And that's exactly what's troubling you now - LOL. (because without that line, you wouldn't have that many problems now).

....and once another case solved by Sherlock Softworkz 😆 😆 😆 😆

brabebhin commented 2 weeks ago

I think before we merge this we should decide on some properties that go in that file. Atm there's just a reference to a file that's in gitignore, so it's not much use.

We should probably keep target target Windows version and min version at the very least.

Maybe some nuget packages as well.

This needs to be investigated further.

JunielKatarn commented 2 weeks ago

I think before we merge this we should decide on some properties that go in that file. Atm there's just a reference to a file that's in gitignore, so it's not much use.

We should probably keep target target Windows version and min version at the very least.

Maybe some nuget packages as well.

This needs to be investigated further.

This was actually my intention; to create the file without any actual content and add the properties in separate subsequent PRs.

And yes, the Target SDK and minimum Windows versions are the first that came to mind. Functionally speaking, this PR is ready to merge for its intended purpose.

The next PR I would send would make its use very clear.

softworkz commented 2 weeks ago

On smaller projects like this one, the history does not really matter much, I never had trouble with it.

If you would have squash-merged my ASS PR into a single commit - for example,, it would be extremely painful for me when I later rebase my branch onto the latest FFmpegInteropX. Especially, when I have made some local modifications that are not in the main repo (this) and there would also have been made some changes in this repo.

It would cause conflicts during rebase of a kind where it's impossible to sort them out just from the conflict/diff. It would be required to resolve the conflicts by reviewing and comparing the histories of each afftected file at both sides.

This is happening because through the squashing you are eliminating the common baseline that Git needs to do it's magic. (in my repo: multiple commits, in this repo: a single commit => no common baseline).

softworkz commented 2 weeks ago

@JunielKatarn - I'm still curious about your setup and workflow - would you mind to explain?

softworkz commented 2 weeks ago

@JunielKatarn - above you had written;

I see you did read this, but you did not understand it. [...] If you carefully read the EditorConfig rules, they already set a specific EOL style for .editorconfig and .gitignore: CRLF via default rules. This causes the files to add CRLF on save but, because UNIX does not use autoCRLF, it will check in the file as is.

(the "this" in the next line is about changing editorconfig to enforce LF for certain files)

Obviously, this won't be a problem on Windows if autoCRLF is on. Setting these two files to LF affects you in no imaginable way and prevents anyone using non-Windows environments editing this project from running into issues.

As promised, here's my response in video form because it seems I had previously failed to explain it in an understandable way (not only in this but also in the preceeding PR which would have applied this to even more files):

NOTE: You need to manually enable the audio here on GitHub

https://github.com/user-attachments/assets/0a6ab318-51f5-4c87-b5a3-287961d62924

https://github.com/user-attachments/assets/d1c1c585-8dd7-4294-8e92-ae3bb3d0f1aa