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

IOS-5453: Add SPM support #333

Merged
merged 20 commits into from
Dec 29, 2023
Merged

Conversation

m3g0byt3
Copy link
Contributor

@m3g0byt3 m3g0byt3 commented Dec 24, 2023

IOS-5453

Старался сделать минимальные изменения структуры папок и файлов, в принципе получилось, единственное что пришлось перенести - это строки локализации.

В таргете юнит-тестов пришлось сделать правки из-за особенности работы SPM с ресурсами, но в итоге все тесты работают.

Example project по прежнему работает через поды, предлагаю обсудить - стоит ли перевести его на SPM или нет? Может вообще сделать два экзампла с общей логикой - один через pods, другой через SPM?

@m3g0byt3 m3g0byt3 added the WIP label Dec 24, 2023
@m3g0byt3 m3g0byt3 changed the title IOS-5453: Add spm support IOS-5453: Add SPM support Dec 24, 2023
s.pod_target_xcconfig = {
'SWIFT_INCLUDE_PATHS' => '$(PODS_TARGET_SRCROOT)/TangemSdk/**',
'OTHER_CFLAGS' => '-pedantic -Wall -Wextra -Wcast-align -Wnested-externs -Wshadow -Wstrict-prototypes -Wno-shorten-64-to-32 -Wno-conditional-uninitialized -Wno-unused-function -Wno-long-long -Wno-overlength-strings -O3 -Wundef -Wreserved-identifier -fvisibility=hidden',
'OTHER_CFLAGS' => '-Wpedantic -Wall -Wextra -Wcast-align -Wnested-externs -Wshadow -Wstrict-prototypes -Wno-shorten-64-to-32 -Wno-conditional-uninitialized -Wno-unused-function -Wno-long-long -Wno-overlength-strings -Wundef -Wreserved-identifier -O3 -fvisibility=hidden',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Вопрос, насколько нужно переносить все эти флаги в SPM?

Большинство из них как я понимаю для косметики (-Wsomething)
А нужную оптимизацию (-O2 или -O3) по идее SPM и так задаст в релизном билде

Copy link
Collaborator

Choose a reason for hiding this comment

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

без каких-то флагов были ошибки/ворнинги.

@@ -7,48 +7,48 @@
#

Pod::Spec.new do |s|
s.name = 'TangemSdk'
Copy link
Contributor Author

@m3g0byt3 m3g0byt3 Dec 25, 2023

Choose a reason for hiding this comment

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

В подспеке без изменений, просто немного поправил форматирование

@@ -1,90 +0,0 @@
/*****************************************************************************************************
Copy link
Contributor Author

@m3g0byt3 m3g0byt3 Dec 25, 2023

Choose a reason for hiding this comment

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

С этим кодом очень забавная история - это по сути кодогенератор, юзаемый при сборке либы secp256k1 для того, чтобы нагенерить и записать в src/precomputed_ecmult.c видимо более оптимальные(?) для конкретной архитектуры векторы:

bitcoin-core/secp256k1#1280

$ ./autogen.sh
$ mkdir ../build && cd ../build
$ ../secp256k1/configure
$ make precomp

bitcoin-core/secp256k1#1281 (comment)

If generate prebuilt sources manually, is it just like that:

gcc src/precompute_ecmult.c -o precompute_ecmult && ./precompute_ecmult
gcc src/precompute_ecmult_gen.c -o precompute_ecmult_gen && ./precompute_ecmult_gen

Именно поэтому в этом файле есть main (собственно в нем и происходит вся кодогенерация в другой сишный файл, src/precomputed_ecmult.c), чтобы его можно было запустить в процессе сборки либы.

Этот файл никак не используется в либе, а наличие в нем main вызывает ошибку дублирующих символов - поэтому я предлагаю его удалить.


Сейчас при подключении TangemSDK и через cocoapods и через SPM не производится это генерирование векторов при сборке.

Если нам в secp256k1 действительно есть необходимость в генерировании векторов при сборке (не знаю насколько это в целом будет работать при сборке под iOS на mac) - то надо или делать отдельный
build step и нем запускать этот файл или апгрейдить саму либу secp256k1, т.к. в bitcoin-core/secp256k1#988 ушли от необходимость делать precompute

Copy link
Collaborator

@tureck1y tureck1y Dec 26, 2023

Choose a reason for hiding this comment

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

Думаю нужно будет написать нам внутренний гайд по обновлению secp256k1 либы

  • переименование основного файла, чтобы ушли ошибки сборки через девелоперские поды
  • правки отсюда a7cacbc
  • удаление файла precompute_ecmult.c

Copy link
Collaborator

Choose a reason for hiding this comment

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

Добавил в notion

// Copyright © 2020 Tangem AG. All rights reserved.
//

import Foundation
Copy link
Contributor Author

@m3g0byt3 m3g0byt3 Dec 25, 2023

Choose a reason for hiding this comment

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

Тут и в AccessCodeRepository.swift без явного импорта не собиралось в SPM, видимо более строгие правила насчет неявных импортов

Comment on lines +146 to +148
/// SPM preserves folder structure for resources, unlike Cocoapods.
/// Therefore, a full file path with all intermediate directories must be constructed.
private func filePath(forResource resource: String) -> String {
Copy link
Contributor Author

@m3g0byt3 m3g0byt3 Dec 25, 2023

Choose a reason for hiding this comment

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

There are two ways to declare a resource:

A process rule: Applies platform specific rules to the resource such as compressing png files. If there are no specific rules for a resource the file is copied to the top-level directory of the bundle. You can specify a directory to have Xcode process the contents. The directory structure is not preserved.

A copy rule: Copies files untouched. If you pass a directory the contents are copied preserving the sub-directory structure.

copy rule к сожалению не настраиваемое, если не указывать каждый файлик по отдельности - то структура папок сохранится. Cocoapods же сваливает все в один бандл

Поэтому нужен этот хелпер

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Стринги все-таки пришлось перенести в отдельную папку на один уровень ниже, т.к. иначе приходится прописывать файл для каждой локали (en, ru, etc) в манифесте явно как ресурс - и соотв легко забыть это сделать, когда будет добавлен новый язык перевода

Comment on lines -2291 to +2305
IPHONEOS_DEPLOYMENT_TARGET = 12.0;
IPHONEOS_DEPLOYMENT_TARGET = 11.0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Т.к. в спеке и манифесте все еще стоит min target 11.0 - то поправил это и в файле проекта (правда в Xcode 15 уже нет поддержки iOS 11)

@@ -345,25 +347,8 @@ class JSONRPCTests: XCTestCase {
XCTAssertEqual(resultData.utf8String!.lowercased(), resultJsonData.utf8String!.lowercased())
}

private func testMethodRequest(name: String) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

что-то не юзаемое

@m3g0byt3 m3g0byt3 marked this pull request as ready for review December 25, 2023 09:03
@m3g0byt3 m3g0byt3 requested review from tureck1y and a team as code owners December 25, 2023 09:03
@m3g0byt3 m3g0byt3 removed the WIP label Dec 25, 2023
@tureck1y
Copy link
Collaborator

Может вообще сделать два экзампла с общей логикой - один через pods, другой через SPM?

Было было идеально, заодно сможем что-то проверять таким образом

Copy link
Collaborator

@tureck1y tureck1y left a comment

Choose a reason for hiding this comment

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

Обсудили, example добавим отдельно

@tureck1y tureck1y merged commit 342eacb into develop Dec 29, 2023
3 checks passed
@tureck1y tureck1y deleted the feature/IOS-5453-add-spm-support branch December 29, 2023 11:53
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.

2 participants