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

Support Artifact Bundle #1388

Merged
merged 11 commits into from Feb 13, 2024
Merged

Support Artifact Bundle #1388

merged 11 commits into from Feb 13, 2024

Conversation

freddi-kit
Copy link
Collaborator

Motivation

SE-0305 is introduced and it is now important for build tools.

Artifact Bundle helps to support Artifact bundle to not take time to making build binary.

I developed new OSS tool named ArtifactBundleGen to create it easily.

Changes

  • Add Artifact Bundle creation flow

Test

Run make archive

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Owner

@yonaskolb yonaskolb left a comment

Choose a reason for hiding this comment

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

Thanks @freddi-kit.

What is the story around the SettingPresets assets? I don't think these are being bundled with the artifact. I assume we need to move these to package resources first.

As an aside, would love the github release process to be automated at some point.

Makefile Outdated
@@ -48,3 +48,7 @@ brew:

archive: build
./scripts/archive.sh "$(EXECUTABLE_PATH)"
swift package plugin --allow-writing-to-package-directory generate-artifact-bundle \
--package-version $(VERSION) \
--executable-name xcodegen \
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
--executable-name xcodegen \
--executable-name $(EXECUTABLE_NAME) \

@freddi-kit
Copy link
Collaborator Author

What is the story around the SettingPresets assets? I don't think these are being bundled with the artifact. I assume we need to move these to package resources first.

I support it in this commit, could you help checking it? 🙇

As an aside, would love the github release process to be automated at some point.

I have idea to do it, let me do it in other PR 👍

@freddi-kit freddi-kit marked this pull request as draft August 17, 2023 05:57
@freddi-kit
Copy link
Collaborator Author

i noticed i need to sereval things for support resource please wait a moment

@freddi-kit freddi-kit marked this pull request as ready for review September 6, 2023 09:23
@freddi-kit
Copy link
Collaborator Author

@yonaskolb
512ec95
Now SettingPresets can be loaded from artifact bundle, please check it

Package.swift Outdated
@@ -2,6 +2,13 @@

import PackageDescription

let macOSOnlyDependency: [Package.Dependency]
#if !os(Linux)
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason not to do a mac check here instead to be more explicit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In CI machine, we cannot build my tools, I'm checking the reason

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@@ -0,0 +1 @@
../../SettingPresets/
Copy link
Owner

Choose a reason for hiding this comment

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

What does this file do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is symbolic link for SeetingPresets in root to detect it as bundle file for XcodeGenKit. Package.swift cannot find it if we don't have it in root of target source file here https://github.com/yonaskolb/XcodeGen/pull/1388/files#diff-f913940c58e8744a2af1c68b909bb6383e49007e6c5a12fb03104a9006ae677eR46

We also cannot set it as .copy("../../SettingPresets")

@freddi-kit
Copy link
Collaborator Author

@yonaskolb Linux issue is solved, please check again

Makefile Show resolved Hide resolved
@roman-aliyev
Copy link

@yonaskolb @freddi-kit We have recently solved the same task. See mac-cain13/R.swift#838 . May be this will help you.

@freddi-kit
Copy link
Collaborator Author

freddi-kit commented Sep 25, 2023

@yonaskolb do you have any concern? I also agree with @roman-aliyev 's way

Comment on lines +205 to +207
if let moduleResourcePath = Bundle.module.path(forResource: "SettingPresets", ofType: nil) {
possibleSettingsPaths.append(Path(moduleResourcePath) + "\(path).yml")
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@roman-aliyev If @yonaskolb agree with your way, please open PR and do not forget to add this code and copy script SettingPresets to bundle.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mat1th
Copy link
Contributor

mat1th commented Jan 5, 2024

Is there any update on this PR? I would like to use XcodeGen from a swiftPackage.

@freddi-kit
Copy link
Collaborator Author

@yonaskolb could you help checking it again? 🙇

Copy link
Owner

@yonaskolb yonaskolb left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @freddi-kit 🙏

@yonaskolb yonaskolb merged commit 2c15007 into master Feb 13, 2024
4 checks passed
@yonaskolb yonaskolb deleted the aritfact-bundle branch February 13, 2024 09:48
@kkebo
Copy link

kkebo commented Feb 14, 2024

I created a sample project. I hope this will be helpful for someone.

https://github.com/kkk669/xcodegen-artifactbundle-demo/

yonaskolb added a commit that referenced this pull request Feb 15, 2024
This reverts commit 2c15007.

# Conflicts:
#	CHANGELOG.md
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

5 participants