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

Fix incorrect formatting of build settings with modifiers #706

Merged
merged 3 commits into from
Jan 13, 2020

Conversation

revolter
Copy link
Contributor

Fixes #666 😈

@revolter
Copy link
Contributor Author

Should I add a test case for this?

@dnkoutso
Copy link
Contributor

Yes please!

@revolter
Copy link
Contributor Author

Done!

@revolter
Copy link
Contributor Author

What's the issue?

@dnkoutso
Copy link
Contributor

Something is wrong with build on "xit or fit left in tests (spec/project/object/build_configuration_spec.rb)"

LGTM otherwise.

@revolter
Copy link
Contributor Author

I fixed it, but it's weird that it was already in master but the last commits on master are green.

@dnkoutso
Copy link
Contributor

Hm maybe danger was updated. If the test was disabled we can even delete it as it probably has been disable for a while.

@revolter
Copy link
Contributor Author

I think that it was only a mistake, and we actually do need those tests. They were added in 7b5c780.

@revolter
Copy link
Contributor Author

Yeah, it looks like the tests are indeed wrong.

@revolter
Copy link
Contributor Author

I think that the bug that the tests are trying to prevent still exists and will need to be fixed in the future, but that doesn't have anything to do with my changes, so if all is good, this should be merged.

@revolter
Copy link
Contributor Author

@dnkoutso, Could you please merge this? 😃

@dnkoutso
Copy link
Contributor

/?hmmm I'd like to fix the build first. Maybe in a separate PR

@revolter
Copy link
Contributor Author

Will you or @segiddins take care of it? And, when will be the next release? I'm worried that it will take a lot of time until I get this change, as it has to be released in a new Xcodeproj version, then create a PR in CocoaPods, then get it merged, then wait for a new CocoaPods release 😟

@dnkoutso
Copy link
Contributor

You can use Bundler https://bundler.io/v1.12/git.html to pin to your fork or branch if this change is blocking you and you don't want to wait for a release, which is understandable.

@revolter
Copy link
Contributor Author

revolter commented Sep 4, 2019

Wouldn't be better if this was merged now? As you're obviously very busy and you won't ever fix those old failing checks unrelated to this change.

Also, I think that @segiddins is the most able to solve it.

@barakwei
Copy link
Contributor

barakwei commented Sep 5, 2019

Can this be merged? Looks like tests are passing, and there's some unrelated issue.

@segiddins
Copy link
Member

Danger has failed this build. 
Found 1 error.

@barakwei
Copy link
Contributor

barakwei commented Oct 7, 2019

Can we please move the forward somehow? it seems like a CI failure to me and not something in the PR. Can we run CI again?

@segiddins
Copy link
Member

Re-kicked CI

@revolter revolter force-pushed the patch-1 branch 2 times, most recently from 9add8cb to d151d71 Compare November 5, 2019 08:59
@revolter
Copy link
Contributor Author

revolter commented Nov 5, 2019

Ok, so in other PRs, there's:

Danger does not have write access to the PR to set a PR status.

Sorry, but, this is bullshit! So my PR was not merged for 3 months because of an error 100% not caused by me, that is not catched in other PRs because of Danger not running at all!

Again, error source:

https://github.com/CocoaPods/Xcodeproj/blame/2bc4222ff4664386363cda25c56d2b54c20d565e/spec/project/object/build_configuration_spec.rb#L118-L122

@revolter
Copy link
Contributor Author

revolter commented Nov 5, 2019

So knowing that it will take a looot of time until this gets into CocoaPods, please merge it as soon as possible, and try to make a new release containing this, again, as soon as possible.

@revolter
Copy link
Contributor Author

revolter commented Nov 5, 2019

Also, is it ok that DANGER_GITHUB_API_TOKEN can be found in https://api.travis-ci.org/v3/job/607538923/log.txt?

@segiddins
Copy link
Member

cc @orta on the danger issue

@segiddins
Copy link
Member

@revolter

Sorry, but, this is bullshit

I'm sorry, but this attitude is not acceptable. CocoaPods is maintained by people in their spare time, and complaining rudely about failing builds or slow release cycles will not make anything happen faster.

@revolter
Copy link
Contributor Author

revolter commented Nov 7, 2019

@segiddins, You're right, I'm sorry for using that word, but I consider that I did everything I could, and the only thing I needed for you was to merge it, nothing more, no extra work.

complaining rudely about failing builds or slow release cycles will not make anything happen faster.

I did not do this. I complained about ignoring my "research" related to the issue, that concluded in the fact that it was 100% not introduced by me in this PR.
And I surely didn't complain about slow release cycles, I completely understand it, it was provided as an extra reason of merging my work. I waited 3 months, so I can wait more.
Also, not only did I not complain, I also spent time to look into it, git blame it, try to fix them myself, even if completely unrelated, and I was met with rejections without a reason (1, 2) or simply ignored (1, 2).

So, again, I know a little about maintaining open source projects, I completely understand the huge effort and time spent by the maintainers, and I have no problem whatsoever to do everything I can to make your job easier.

@revolter
Copy link
Contributor Author

revolter commented Nov 7, 2019

Also, it's weird that there is a Danger check with is green, but the CI check fails because of Danger.

@orta
Copy link
Member

orta commented Nov 7, 2019

The message is here: #706 (comment)

Screen Shot 2019-11-07 at 8 48 11 AM

if it's fixed upstream, maybe rebase? that error just exists in the current code from the sounds of things

@revolter
Copy link
Contributor Author

revolter commented Nov 7, 2019

@orta, Yes, xit is the cause. The problem is that other (some) PRs don't trigger this, and this is logged:

Danger does not have write access to the PR to set a PR status.

@orta
Copy link
Member

orta commented Nov 7, 2019

The dangerfile only looks at modified files for the xit/fit checks, and this PR includes those changes - and don't worry about the status update message, that's because the CI version doesn't have write access to the repo, which we want.

@revolter
Copy link
Contributor Author

revolter commented Nov 7, 2019

Got it, thanks a lot!

@revolter
Copy link
Contributor Author

revolter commented Nov 7, 2019

@orta, What about #706 (comment)?

@orta
Copy link
Member

orta commented Nov 7, 2019

@revolter
Copy link
Contributor Author

As #727 was merged, I rebased this branch and it builds successfully, so this is now ready for merge.

@segiddins segiddins merged commit 70d114e into CocoaPods:master Jan 13, 2020
@revolter revolter deleted the patch-1 branch January 13, 2020 22:36
@revolter
Copy link
Contributor Author

To use this fix before it gets officially released, add an explicit xcodeproj gem dependency in your Gemfile like:

# frozen_string_literal: true

source 'https://rubygems.org' do
    gem 'cocoapods'
    gem 'xcodeproj', git: 'https://github.com/CocoaPods/Xcodeproj.git', ref: '70d114e'
end

@revolter
Copy link
Contributor Author

revolter commented Feb 5, 2020

To use this fix after it was officially released, simply run bundle update xcodeproj.

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.

Incorrectly changing the formatting of the GCC_PREPROCESSOR_DEFINITIONS config
5 participants