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.

Sign up for GitHub

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

Jump to bottom

Bug fixes for mac OS #6207

Merged
merged 3 commits into from
Dec 11, 2023
Merged

Bug fixes for mac OS #6207

merged 3 commits into from
Dec 11, 2023

Conversation

camilasan
Copy link
Member

No description provided.

@codecov Codecov
Copy link

codecov bot commented Nov 7, 2023 •
edited
Loading

Codecov Report

Merging #6207 (f398bc4) into master (3dc583c) will decrease coverage by 0.03%.
Report is 48 commits behind head on master.
The diff coverage is n/a.

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

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6207      +/-   ##
==========================================
- Coverage   60.79%   60.76%   -0.03%     
==========================================
  Files         145      145              
  Lines       18836    18846      +10     
==========================================
+ Hits        11451    11452       +1     
- Misses       7385     7394       +9

see 8 files with indirect coverage changes

@camilasan camilasan force-pushed the bugfix/macos-legacy branch 2 times, most recently from 9d7ff9d to 27b615d Compare November 8, 2023 12:33
@camilasan camilasan marked this pull request as ready for review November 8, 2023 12:33
claucambra
claucambra reviewed Nov 8, 2023
View reviewed changes
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.

Are we sure we want to go back to 10.13? Our previous target was 10.14 (we have components that depend on 10.14, mainly notifications)

@camilasan
Copy link
Member Author

camilasan commented Nov 8, 2023 •
edited
Loading

Are we sure we want to go back to 10.13? Our previous target was 10.14 (we have components that depend on 10.14, mainly notifications)

See internal chat ;), it is customer related.
Could we disable the notifications component? Would that even be possible?

mgallien
mgallien requested changes Nov 8, 2023
View reviewed changes
src/CMakeLists.txt Outdated Show resolved Hide resolved
@claucambra claucambra changed the title Fixes for macos legacy Reimplement support for macOS 10.13 Nov 13, 2023
@claucambra claucambra changed the title Reimplement support for macOS 10.13 Reimplement support for macOS 10.13+ Nov 13, 2023
github-actions[bot]
github-actions bot requested changes Nov 13, 2023
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/gui/systray_mac_usernotifications.mm Outdated Show resolved Hide resolved
src/gui/systray_mac_common.mm Outdated Show resolved Hide resolved
@camilasan camilasan force-pushed the bugfix/macos-legacy branch 2 times, most recently from e352b2f to ee4c09b Compare November 13, 2023 11:02
@marcotrevisan marcotrevisan mentioned this pull request Nov 14, 2023
Open
8 tasks
@camilasan camilasan force-pushed the bugfix/macos-legacy branch 6 times, most recently from 2826f42 to 931889d Compare December 1, 2023 21:04
@camilasan camilasan changed the title Reimplement support for macOS 10.13+ Bug fixes for mac OS Dec 4, 2023
@camilasan
Copy link
Member Author

/backport to stable-3.11

mgallien
mgallien requested changes Dec 6, 2023
View reviewed changes
CMakeLists.txt Show resolved Hide resolved
src/csync/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt
Comment on lines +110 to +111
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,-ld_classic")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,-ld_classic")
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this not be set only when built with a recent enough XCode ?

Copy link
Member Author

Choose a reason for hiding this comment

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

considering the tests I did and the customer's, this is fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes
I thought that was causing an issue with the macOS CI ?

Copy link
Member Author

Choose a reason for hiding this comment

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

is it?
these are the errors from Craft:
Failed to find jom and
FileNotFoundError: [Errno 2] No such file or directory: 'meson'

Copy link
Member Author

Choose a reason for hiding this comment

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

it is actually the same at https://github.com/nextcloud/desktop/actions/runs/7128782457/job/19411414926?pr=6247

@camilasan camilasan force-pushed the bugfix/macos-legacy branch 2 times, most recently from 89f1b68 to b19cd00 Compare December 11, 2023 09:09
mgallien
mgallien requested changes Dec 11, 2023
View reviewed changes
src/csync/CMakeLists.txt Outdated Show resolved Hide resolved
Camila and others added 3 commits December 11, 2023 11:50
Fix crash on mac OS < 13.
3c51c9e 
It's caused by the new optimized Xcode 15 linker when linking static libraries.
source: https://crystalidea.com/blog/qt-apps-crash-when-using-xcode-15

Signed-off-by: Camila <[email protected]>
@claucambra
Only set linker flags needed for macOS on Apple
c9b4419 
Signed-off-by: Claudio Cambra <[email protected]>
Use a new FindSQLite3 cmake module.
02e4f05 
Add find sqlite to the root CMakeLists.txt.

Signed-off-by: Camila <[email protected]>
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-6207-02e4f05358f70a1c9c144e657e2b1e4a12bfa06f-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.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Dec 11, 2023

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

@mgallien mgallien merged commit d3562e6 into master Dec 11, 2023
10 of 12 checks passed
@mgallien mgallien deleted the bugfix/macos-legacy branch December 11, 2023 14:32
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Dec 11, 2023
Merged
@mgallien mgallien added this to the 3.12.0 milestone Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claucambra claucambra claucambra left review comments

@github-actions GitHub Actions github-actions requested changes

@mgallien mgallien mgallien approved these changes

@allexzander allexzander Awaiting requested review from allexzander

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.12.0
Development

Successfully merging this pull request may close these issues.
4 participants
@camilasan @nextcloud-desktop-bot @mgallien @claucambra