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

makes Qt WebEngine optional only on macOS #4875

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

mgallien
Copy link
Collaborator

should avoid distributing broken builds missing out web flow login that
is required by some cusotmers

Signed-off-by: Matthieu Gallien matthieu.gallien@nextcloud.com

@allexzander allexzander self-requested a review August 29, 2022 10:02
@allexzander
Copy link
Contributor

@mgallien I was too quick to approve. It is failing on drone.

Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

Besides the CI failures, why are we separating all these package properties instead of just the webengine? I am not sure we are adding anything with the descriptions except making things longer

@mgallien
Copy link
Collaborator Author

mgallien commented Sep 1, 2022

Besides the CI failures, why are we separating all these package properties instead of just the webengine? I am not sure we are adding anything with the descriptions except making things longer

The goal is to improve the summary at end of configure time when running cmake.
I can also understand your comment.

@mgallien mgallien force-pushed the bugfix/webEngineOptionalOnAppleOnly branch from a841925 to b01309f Compare September 1, 2022 15:40
@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #4875 (b01309f) into master (ee513b6) will increase coverage by 0.06%.
The diff coverage is n/a.

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

@@            Coverage Diff             @@
##           master    #4875      +/-   ##
==========================================
+ Coverage   57.15%   57.22%   +0.06%     
==========================================
  Files         138      138              
  Lines       17135    17146      +11     
==========================================
+ Hits         9794     9811      +17     
+ Misses       7341     7335       -6     
Impacted Files Coverage Δ
src/libsync/owncloudpropagator.h 73.28% <0.00%> (-1.16%) ⬇️
src/libsync/owncloudpropagator.cpp 86.00% <0.00%> (-0.03%) ⬇️
src/libsync/syncengine.cpp 87.22% <0.00%> (+0.02%) ⬆️
src/libsync/vfs/cfapi/cfapiwrapper.cpp 74.25% <0.00%> (+0.05%) ⬆️
src/libsync/propagatedownload.cpp 65.18% <0.00%> (+0.14%) ⬆️
src/libsync/propagateremotemkdir.cpp 65.24% <0.00%> (+0.70%) ⬆️
src/libsync/vfs/cfapi/vfs_cfapi.cpp 87.45% <0.00%> (+1.03%) ⬆️
src/libsync/propagateuploadng.cpp 83.89% <0.00%> (+1.49%) ⬆️

@mgallien
Copy link
Collaborator Author

mgallien commented Sep 7, 2022

Besides the CI failures, why are we separating all these package properties instead of just the webengine? I am not sure we are adding anything with the descriptions except making things longer

The goal is to improve the summary at end of configure time when running cmake. I can also understand your comment.

@claucambra so do you want changes or do you approve ?

@claucambra
Copy link
Collaborator

Besides the CI failures, why are we separating all these package properties instead of just the webengine? I am not sure we are adding anything with the descriptions except making things longer

The goal is to improve the summary at end of configure time when running cmake. I can also understand your comment.

@claucambra so do you want changes or do you approve ?

Besides the CI failures, why are we separating all these package properties instead of just the webengine? I am not sure we are adding anything with the descriptions except making things longer

The goal is to improve the summary at end of configure time when running cmake. I can also understand your comment.

@claucambra so do you want changes or do you approve ?

approved :)

should avoid distributing broken builds missing out web flow login that
is required by some cusotmers

Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
@mgallien mgallien force-pushed the bugfix/webEngineOptionalOnAppleOnly branch from b01309f to b81cdf1 Compare September 8, 2022 08:33
@sonarcloud
Copy link

sonarcloud bot commented Sep 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-4875-b81cdf177d3410db45e6f97eb3575ceddabb73dd-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@mgallien mgallien merged commit 6ab7ada into master Sep 8, 2022
@mgallien mgallien deleted the bugfix/webEngineOptionalOnAppleOnly branch September 8, 2022 13:50
@mgallien mgallien added this to the 3.7.0 milestone Oct 3, 2022
@sseneca
Copy link

sseneca commented Feb 8, 2023

Requiring webengine makes it impossible to build/use the client on libre distributions like Parabola.

Is there a way to build the client without it, since it can mostly work fine without webengine?

@bill-auger
Copy link

does the build strictly require webengine? - the last i remember, it was intentionally made to avoid that a few years ago - there have been multiple discussions and pull requests about this ( #932 #1808 #856 #2204 #3353 ) - there was even a 'no-webengine' branch at one time working toward that goal - why does this keep creeping back in? - did someone leave the barn door open?

@TimB87
Copy link

TimB87 commented Feb 11, 2023

On CRUX I used to make qtwebengine (especially the qt5 version...) optional, see this commit

@mgallien @claucambra Is there any reason why everybody wants to hard depend on that? It's a huge dependency with a lot of pitfalls, it would be great to be able to avoid it. Maybe a flag to optionally disable it?

@mgallien
Copy link
Collaborator Author

important features do not work without QtWebEngine (may be required for users to login to a Nextcloud server)
this is the reason to have this mandatory when feasible to build it

@TimB87
Copy link

TimB87 commented Feb 15, 2023

(may be required for users to login to a Nextcloud server)

May be, but doesn't have to be, correctly?

@TimB87
Copy link

TimB87 commented Feb 15, 2023

a flag to disable qtwebengine requirement would work for me and probably @bill-auger and @sseneca as well, it can still demand it by default. How do you feel about that @mgallien?

@sseneca
Copy link

sseneca commented Feb 15, 2023

may be required for users to login to a Nextcloud server

I've been using the Nextcloud client with my Nextcloud instance for a while now, without qtwebengine installed :) So it works fine for me.

a flag to disable qtwebengine requirement would work for me and probably [...] as well

+1 on this, just having built-in support to opt-out from the webengine dependency would be really nice.

@mgallien
Copy link
Collaborator Author

mgallien commented Feb 17, 2023

a flag to disable qtwebengine requirement would work for me and probably @bill-auger and @sseneca as well, it can still demand it by default. How do you feel about that @mgallien?

may work
will review a PR for that (especially if you ping me about it)
that means for example loosing access to using an existing provider via this first setup wizard feature
image
so getting something like this merged may be a bit more work than just a couple of cmake changes

@TimB87
Copy link

TimB87 commented Feb 17, 2023

I for one can not provide a complete PR for that..

@bill-auger
Copy link

bill-auger commented Feb 22, 2023 via email

@mdkcore0
Copy link

this also breaks building on some targeted architectures on void-linux; can we have a flag to disable the use of webengine?

mdkcore0 added a commit to mdkcore0/void-packages that referenced this pull request Feb 23, 2023
Also:
- Add karchive as dependency [1]
- Add patch to revert mandatory WebEngine dependency [2]

[1] nextcloud/desktop#4768
[2] nextcloud/desktop#4875
mdkcore0 added a commit to mdkcore0/void-packages that referenced this pull request Mar 7, 2023
Also:
- Add karchive as dependency [1]
- Add patch to revert mandatory WebEngine dependency [2]

[1] nextcloud/desktop#4768
[2] nextcloud/desktop#4875
paper42 pushed a commit to void-linux/void-packages that referenced this pull request Mar 9, 2023
Also:
- Add karchive as dependency [1]
- Add patch to revert mandatory WebEngine dependency [2]

[1] nextcloud/desktop#4768
[2] nextcloud/desktop#4875
kaistian pushed a commit to kaistian/void-packages that referenced this pull request Mar 13, 2023
Also:
- Add karchive as dependency [1]
- Add patch to revert mandatory WebEngine dependency [2]

[1] nextcloud/desktop#4768
[2] nextcloud/desktop#4875
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.

8 participants