Skip to content
This repository has been archived by the owner on Apr 2, 2020. It is now read-only.

Apply local llvm/clang/swift patches when using cmake build system #252

Merged
merged 1 commit into from
Sep 27, 2017

Conversation

fjricci
Copy link
Collaborator

@fjricci fjricci commented Sep 27, 2017

This is already supported in the Xcode and bash build scripts, add
to the cmake build scripts for parity.

This is already supported in the Xcode build scripts, add
to the cmake build scripts for parity.
Copy link
Member

@jimingham jimingham left a comment

Choose a reason for hiding this comment

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

The Xcode build does this job, so it seems appropriate for the cmake build to do so as well.

However, we want to avoid if at all possible having to maintain patchsets against other tools. So while the mechanism for local development purposes is fine, particularly as you are working patches into the other tools, there will be a pretty high bar to actually adding .diff files to the github repository.

@fjricci
Copy link
Collaborator Author

fjricci commented Sep 27, 2017

Yeah, we use the patchset feature for some internal changes and build system peculiarities, but I agree that it doesn't make sense in general for upstream work.

@jasonmolenda
Copy link
Contributor

I've needed to maintain patches to llvm/clang/swift that I couldn't get upstreamed for a short period in the past, I've had similar code on branches to do this too. The main question I have about the patch is the 'patch -p0' - normally git diffs need -p1 don't they? Are the patches from svn, or is there some way to make git diff not do that?

@fjricci
Copy link
Collaborator Author

fjricci commented Sep 27, 2017

Yeah, I've debated switching things to -p1 myself, but generally assumed we chose -p0 for some reason. I'd be happy to put up a diff for -p1 (it would make my life a bit easier for sure)

@fjricci
Copy link
Collaborator Author

fjricci commented Sep 27, 2017

Also, would one of you mind merging this commit? I don't yet have swift commit access

@jasonmolenda jasonmolenda merged commit 330a726 into apple:stable Sep 27, 2017
@jasonmolenda
Copy link
Contributor

ach, I also committed your matching phabracator change on llvm.org, sorry looking at too many different branches today and I forgot that only the github change needed to be committed.

@fjricci fjricci deleted the pathcer branch September 28, 2017 14:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants