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

Do not call ssu to determine OS Version #65

Merged
merged 1 commit into from
Oct 3, 2021

Conversation

nephros
Copy link
Contributor

@nephros nephros commented Sep 28, 2021

See issue #33
https://github.com/sailfishos-patches/patchmanager/issues/33

Concept ported from @elros, ac0991b
https://github.com/elros34/patchmanager/commit/ac0991b1b27df010066fa4e3f8f287c6b1004a3e

@Olf0
Copy link
Contributor

Olf0 commented Sep 29, 2021

Even though you stated to "not speak Qt and C++", you seem to be able to write and express what you want very well. I can barely comprehend most of that, thus a big thank you for adapting elros34's code for PM2.

Code seems fine, but:

  1. this bit, while valid I assume, seems unrelated to the overall change:
    elros34@ac0991b#diff-da6a1e0621bec30e25650901e8a69669572d302bbccc7f82e5fc81f4b90e4e68L312
  2. Thinking of the upcoming sandboxing, how do we feel about reading files from /etc instead of dbus-calling a system service? There are rumors that the latter might break when sandboxing is turned on fully, and the former will break certainly if the app profile doesn't allow it. (There's a separate issue #64 on that though.)
  • For point 1, I also believe it is unrelated.
  • For point 2: Because there is not definite information, how these restrictions are applied, my stance is: "do not care about rumors"; lets test, when the code is out (or more specific information is provided, but that is purely theoretical with Jolla).

For a proper review, I kindly ask @b100dian to do this, because I can only provide uneducated statements (e.g., "The code is looking beautiful, like a dense forest of words; and with syntax hilightning it all becomes so colourful!" 😉).

Copy link
Contributor

@b100dian b100dian left a comment

Choose a reason for hiding this comment

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

Changes look good, I just didn't test them.
In case there are problems with reading directly that file we can always add a dbus method on patchmanager-object to make the query for the UI plugin.

@Olf0
Copy link
Contributor

Olf0 commented Sep 30, 2021

Changes look good, I just didn't test them.

Mmmh, I always reviewed changes by reviewing them. 😜
More seriously, I never tested any changes up to now, before setting "Approved", just reviewed them thoroughly. I may test, if I am very curious / I deem a change very critical / I have time to spoil.

So I do / did not think that any kind of testing is required for approving a MR.
I also believe we should not set the quality-assurance and procedural hurdles too high, as that may stall processing MRs etc.

Do our opinions on this differ (@b100dian sounds a bit like they do)?
Are there some reviewing guidelines, policies or best practices I am not aware of (and which include some testing)?

In this specific case (and many others, when there are non-trivial changes to QML or C++ code), I cannot seriously review due to my lack of knowledge and experience. And I sure will not just hit "Approved" to let things proceed.

So @b100dian, it is either you or potentially nobody who approves this MR.

Copy link
Contributor

@b100dian b100dian left a comment

Choose a reason for hiding this comment

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

This could have been breaking things, that's why the test - I don't test all PRs, of course. The visual ones if I'm more curious:)
Just did a quick run on this and it's fine.
I wasn't able to trigger the original bug just by running ssu re 4.2.0.20 (e.g. change to an non-existant version)

@nephros nephros merged commit d130493 into sailfishos-patches:master Oct 3, 2021
@nephros nephros deleted the issue-33 branch October 3, 2021 07:30
@nephros
Copy link
Contributor Author

nephros commented Oct 3, 2021

Damnit, too soon!

src/bin/patchmanager-daemon/patchmanagerobject.h/cpp must be updated to use the new osVersion actually, otherwise compatibility check on the web catalog does not work.
Sorry about that, fix incoming.

nephros added a commit to nephros/patchmanager that referenced this pull request Oct 3, 2021
commit e546dc0
Author: Peter G <sailfish@nephros.org>
Date:   Sun Oct 3 12:51:06 2021 +0200

    Move from SsuRelease to OsRelease

    should fix some fallout from PR sailfishos-patches#65

    sailfishos-patches#65
nephros added a commit to nephros/patchmanager that referenced this pull request Oct 3, 2021
nephros added a commit to nephros/patchmanager that referenced this pull request Oct 3, 2021
commit e546dc0
Author: Peter G <sailfish@nephros.org>
Date:   Sun Oct 3 12:51:06 2021 +0200

    Move from SsuRelease to OsRelease

    should fix some fallout from PR sailfishos-patches#65

    sailfishos-patches#65
nephros added a commit to nephros/patchmanager that referenced this pull request Oct 3, 2021
commit e546dc0
Author: Peter G <sailfish@nephros.org>
Date:   Sun Oct 3 12:51:06 2021 +0200

    Move from SsuRelease to OsRelease

    should fix some fallout from PR sailfishos-patches#65

    sailfishos-patches#65
nephros added a commit to nephros/patchmanager that referenced this pull request Oct 3, 2021
commit 68bc530
Author: Peter G <sailfish@nephros.org>
Date:   Sun Oct 3 16:52:39 2021 +0200

    fixup! Move from SsuRelease to OsRelease

commit e546dc0
Author: Peter G <sailfish@nephros.org>
Date:   Sun Oct 3 12:51:06 2021 +0200

    Move from SsuRelease to OsRelease

    should fix some fallout from PR sailfishos-patches#65

    sailfishos-patches#65
nephros added a commit to nephros/patchmanager that referenced this pull request Oct 3, 2021
nephros added a commit that referenced this pull request Oct 4, 2021
* Move from SsuRelease to OsRelease

should fix some fallout from PR #65

#65
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.

[PM3 suggestion] Retrieve the installed SFOS version by using 'version', not 'ssu re'
3 participants