Stackdriver / collectd

Stackdriver's monitoring agent based on collectd (http://collectd.org).
https://cloud.google.com/monitoring/agent/
Other
51 stars 15 forks source link

Delete automake/autoconf generated files #113

Closed jkohen closed 6 years ago

jkohen commented 7 years ago

Except those which aren't part of our build system, for instance, libmongoc, because those can't be recreated with our configure script.

I verified that this works (in my workstation) if I download, then do: automake && autoconf && ./configure --enable-write_gcm

jkohen commented 7 years ago

Igor, thanks for the suggestions. This change is intended to remove stale files that lead to a bad build.

Following what clean.sh does requires changes to the packaging files to rebuild the files, and if I understood Dhrupad correctly, the build system failed to reconstruct them properly when he tried that. I would keep the scope of the CL to what it is now, and I'm happy to review follow-ups.

As for deleting individual files you mentioned, have you tested that automake && autoconf restore them, or that they're not important? If you haven't tested it, I would also like not to include them in this CL.

dhrupadb commented 7 years ago

I think we're getting sidetracked, so I'll clarify. This change is not "critical" to ensuring that new files are correctly picked up (i.e. changes from #109 correctly make it into our package). If I remember correctly, this change was to avoid situations in the future where someone not 100% familiar with how things are setup, mistakenly believes that the build will correctly incorporate their changes (and do much needed technical debt cleanup). This is not a blocker for releasing features from #109 . I'm submitting a change to our build system which unblocks the release.

In any case, I vote not to merge this change in any form until we've moved to the new build pipeline. The subtle differences between two paths leads to unnecessary complexity. And given that we're switching soon (order of days), I don't think it's a worthwhile exercise (i.e. loss of engineering time at the moment vs. the relative gain in reducing technical debt from a system about to be deprecated).

jkohen commented 7 years ago

It's true that the packaging fixes will fix this for packaging, but that only means that developers may be testing with a different build system than the build environment. Seeing how the stale files broke the agent packages and took several SWEs in our team significant time to debug, I have to disagree with your assessment.

But since you're the packaging owner, all I can ask is to make sure this isn't a problem in the new packing system in the next few days.

On Thu, Oct 5, 2017 at 11:12 AM Dhrupad Bhardwaj notifications@github.com wrote:

I think we're getting sidetracked, so I'll clarify. This change is not "critical" to ensuring that new files are correctly picked up (i.e. changes from #109 https://github.com/Stackdriver/collectd/pull/109 correctly make it into our package). If I remember correctly, this change was to avoid situations in the future where someone not 100% familiar with how things are setup, mistakenly believes that the build will correctly incorporate their changes (and do much needed technical debt cleanup). This is not a blocker for releasing features from #109 https://github.com/Stackdriver/collectd/pull/109 .

In any case, I vote not to merge this change in any form until we've moved to the new build pipeline. The subtle differences between two paths leads to unnecessary complexity. And given that we're switching soon (order of days), I don't think it's a worthwhile exercise (i.e. loss of engineering time at the moment vs. the relative gain in reducing technical debt from a system about to be deprecated).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Stackdriver/collectd/pull/113#issuecomment-334496240, or mute the thread https://github.com/notifications/unsubscribe-auth/AG0TaIWfv6noqz5RRgH12TpUrFizO9zAks5spPHkgaJpZM4Psz3k .

igorpeshansky commented 7 years ago

And to respond to @jkohen's earlier question: yes, I've verified that running build.sh will reconstruct all of the files I've suggested deleting.

jkohen commented 7 years ago

Igor, I know that, but we don't run build.sh in the packaging scripts. Dhrupad tested it in the build environment, and it failed. I don't have the error message handy.

On Thu, Oct 5, 2017 at 1:19 PM igorpeshansky notifications@github.com wrote:

And to respond to @jkohen https://github.com/jkohen's earlier question: yes, I've verified that running build.sh will reconstruct all of the files I've suggested deleting.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/Stackdriver/collectd/pull/113#issuecomment-334533107, or mute the thread https://github.com/notifications/unsubscribe-auth/AG0TaDSnCHR-5rXRzy_jpvEjRv_qKBfYks5spQ-5gaJpZM4Psz3k .

igorpeshansky commented 7 years ago

@dhrupadb can confirm, but, as far as I know, we've just switched to running build.sh.

dhrupadb commented 7 years ago

We do. however this PR is breaking on the system. I'm figuring out why. If we're doing it right let's just delete everything clean.sh gets rid of and I'll debug from there, given that it's broken either way.

jkohen commented 7 years ago

Let's avoid the broken telephone. Dhrupad, can you take care of both this CL and fixing the build system at your own timing?

On Thu, Oct 5, 2017 at 1:57 PM Dhrupad Bhardwaj notifications@github.com wrote:

We do. however this PR is breaking on the system. I'm figuring out why. If we're doing it right let's just delete everything clean.sh gets rid of and I'll debug from there, given that it's broken either way.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Stackdriver/collectd/pull/113#issuecomment-334543366, or mute the thread https://github.com/notifications/unsubscribe-auth/AG0TaKQ5m548Ijx84l1lQzTJGVA2wneRks5spRhygaJpZM4Psz3k .

dhrupadb commented 7 years ago

Yes sounds good. Thanks!

dhrupadb commented 7 years ago

This PR is currently infeasible. Closing.

igorpeshansky commented 7 years ago

I would suggest just leaving this around and building on it later. All we'll do eventually is delete more files, right?