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

Deprecate wrap-redirect and stop creating them #9423

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xclaesse
Copy link
Member

It has been a source of confusion to many users and are hard to keep in
gitignore (https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1140).
In general we should avoid creating unexpected files in user's source
tree.

Originally they served 2 purposes:

  • Inform user where a wrap that has been auto promoted comes from. This
    information is already in the log, and feedback shows that it created
    more confusion than actual useful information.
  • Ensure that subsequent builds will use the same wrap in case the order
    in which subprojects are configured changes. This is potentially wrong
    in the case the user pruposedly change the order in which their
    subprojects are configured. First one wins is an established policy,
    people could be counting on it.

Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

Your logic seems sound to me, but I'm not super familiar with this code, consider this an ack

@codecov
Copy link

codecov bot commented Oct 21, 2021

Codecov Report

Merging #9423 (aefaff1) into master (4934b99) will decrease coverage by 4.72%.
The diff coverage is n/a.

❗ Current head aefaff1 differs from pull request most recent head 8542caa. Consider uploading reports for the commit 8542caa to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9423      +/-   ##
==========================================
- Coverage   66.55%   61.83%   -4.73%     
==========================================
  Files         402      201     -201     
  Lines       85504    42674   -42830     
  Branches    18811     9311    -9500     
==========================================
- Hits        56911    26386   -30525     
+ Misses      24202    14192   -10010     
+ Partials     4391     2096    -2295     
Impacted Files Coverage Δ
dependencies/dev.py 59.64% <0.00%> (-0.18%) ⬇️
wrap/wrap.py 61.23% <0.00%> (-0.08%) ⬇️
cmake/interpreter.py 9.79% <0.00%> (ø)
dependencies/__init__.py 100.00% <0.00%> (ø)
mesonbuild/compilers/mixins/gnu.py
mesonbuild/dependencies/__init__.py
mesonbuild/dependencies/dev.py
mesonbuild/wrap/wrap.py
mesonbuild/linkers/linkers.py
mesonbuild/mconf.py
... and 196 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4934b99...8542caa. Read the comment docs.

@xclaesse
Copy link
Member Author

Also, I don't think it has ever been documented, can't find anything about it.

@lgtm-com
Copy link

lgtm-com bot commented Oct 21, 2021

This pull request introduces 1 alert when merging 8352c55 into 939394e - view on LGTM.com

new alerts:

  • 1 for Unused import

Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

I agree with the motivation and from a quick eyeball the changes look correct as well.

@xclaesse
Copy link
Member Author

I guess this could be a bit much to push in 0.60.0? Personally I would still like to have it.

@eli-schwartz
Copy link
Member

Let's not come in the middle of an RC freeze and do policy changes which deprecate features that were deemed bad. :D I don't think that one can be reasonably called "a small bugfix which we'd end up backporting to 0.60.1 anyway"...

@jpakkane
Copy link
Member

First one wins is an established policy, people could be counting on it.

No, this is just wrong, because you can change the order in unexpected ways. For example if you update your subprojects they might add new dependencies without telling anyone and this may change where the dependency comes from. This is poor usability.

One of the main points of the design was that in order to answer the question "what dependencies is this project currently using and where they come from" you only need to look into one, and only one, place. This is also what we have said in public (and presumably also in our public docs). Changing it would be bad.

@mon
Copy link
Contributor

mon commented Oct 24, 2021

For example if you update your subprojects they might add new dependencies without telling anyone and this may change where the dependency comes from

If a subproject deletes a dep, the wrap redirect becomes invalid anyway. If you're updating subprojects you should be aware things can change.

Questions such as "what deps are in use" should be trivially answerable with introspect.

The current behaviour breaks the idea that meson shouldn't be meddling with the filesystem outside the build folder (wraps in general do this anyway by cloning into ./subprojects, but feel sensible enough).

If this PR is unmergable, at the very least it would be nice to name the wrap redirects in a way that can be added as a wildcard to .gitignore.

@xclaesse
Copy link
Member Author

xclaesse commented Oct 29, 2021

For example if you update your subprojects they might add new dependencies without telling anyone and this may change where the dependency comes from. This is poor usability.

But then, if you rebuild after "git clean" that would remove those redirect wraps, it will change things too. That's poor usability as well. That's even worse because your builds are not reproducible (the existence of not tracker local files change the outcome) in a super subtle way that nobody will understand.

Also, the change in the order may very well be intentional, and the presence of local files messing with that could be really hard to understand.

I know it's not perfect, but we now actually log the order in which subprojects have been pulled. For example:

Executing subproject gst-plugins-bad:openh264:gtest

That tells it is gst-plugins-bad that pulled openh264 that itself pulled gtest. A bit buried in the logs, I would like to find a way to make that more visible. But IMHO redirect wraps was a bad idea for the job, it has I think more cons and pros.

@nirbheek nirbheek modified the milestones: 0.61.0, 0.62.0 Jan 10, 2022
It has been a source of confusion to many users and are hard to keep in
gitignore (https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1140).
In general we should avoid creating unexpected files in user's source
tree.

Originally they served 2 purposes:
- Inform user where a wrap that has been auto promoted comes from. This
  information is already in the log, and feedback shows that it created
  more confusion than actual useful information.
- Ensure that subsequent builds will use the same wrap in case the order
  in which subprojects are configured changes. This is potentially wrong
  in the case the user pruposedly change the order in which their
  subprojects are configured. First one wins is an established policy,
  people could be counting on it.
@jpakkane
Copy link
Member

jpakkane commented Mar 7, 2022

But then, if you rebuild after "git clean" that would remove those redirect wraps, it will change things too.

If you run a git clean you are already expecting things to change in a major way (i.e. "go back to last known good configuration"). It really is no different to running something like rm -rf subprojects/foo*.

@mon
Copy link
Contributor

mon commented Mar 7, 2022

If you run a git clean you are already expecting things to change in a major way

Isn't there a similar expectation that things will change in a major way when you update your subprojects?

As far as I can tell this "updating subprojects will maybe change deps" is the only con to a wrap-less subproject resolver. Does it outweigh the pros?

@xclaesse
Copy link
Member Author

btw, to improve logging of which subproject pulled which other subproject, there is #7829

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.

None yet

6 participants