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

Generate binaries only if there are changes in src code. #4762

Merged
merged 1 commit into from
Jan 6, 2020

Conversation

NevilleC
Copy link
Collaborator

@NevilleC NevilleC commented Dec 29, 2019

Changes I am making:

  1. The target .gopathok was listed in .PHONY which looks wrong as it regenerates .gopathok every time we re-run it, which was a part of the issue make install should not make binaries if already exist #4367. I removed it to avoid that. If .gopathok is present', makefile should not need to rerun it.
  2. Ensure the binaries are created only if they don't exist by renaming podman to bin/podman and podman-remote to bin/podman-remote.
  3. Add a SOURCES = $(shell find . -name "*.go") and put it as a dependency of the podman binaries target. It allows us to re-generate the binaries only when there is a change in the source files. The downside is it increases the running time of the command that generates them (20 seconds on my virtual machine running Centos 7). If this is a problem, we could introduce a hidden file that would list all the files to track, that would need to be updated only when a dev is introducing new files.

We can now run make && sudo make install without error, so if this solution is accepted, I believe it should solve the issue listed here.

Closes #4367

Signed-off-by: NevilleC [email protected]

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S labels Dec 29, 2019
@openshift-ci-robot
Copy link
Collaborator

Hi @NevilleC. Thanks for your PR.

I'm waiting for a containers member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@NevilleC NevilleC force-pushed the nc-issue4367 branch 2 times, most recently from 1c66ba2 to dc87656 Compare December 29, 2019 20:25
@NevilleC NevilleC changed the title [Issue #4367] Generate binaries only if they are changes in src code. WIP: [Issue #4367] Generate binaries only if they are changes in src code. Dec 29, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 29, 2019
dependencies/analyses/README.md Outdated Show resolved Hide resolved
dependencies/analyses/README.md Outdated Show resolved Hide resolved
@NevilleC NevilleC changed the title WIP: [Issue #4367] Generate binaries only if they are changes in src code. [Issue #4367] Generate binaries only if they are changes in src code. Dec 29, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 29, 2019
@NevilleC NevilleC changed the title [Issue #4367] Generate binaries only if they are changes in src code. [Issue #4367] Generate binaries only if there are changes in src code. Dec 29, 2019
@NevilleC
Copy link
Collaborator Author

/retest

@openshift-ci-robot
Copy link
Collaborator

@NevilleC: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@rhatdan
Copy link
Member

rhatdan commented Dec 31, 2019

LGTM
@edsantiago @mheon PTAL

@rhatdan
Copy link
Member

rhatdan commented Dec 31, 2019

Thanks @NevilleC nice work.

@edsantiago
Copy link
Member

I have a nagging memory that we've tried this before and it broke the specfile build. I can't find the relevant code or emails, and git blame wasn't helpful, and I have commitments soon that prevent me from pursuing right now. Commenting in case it rings a bell for someone else. It does look good, I just think we need to make sure it works with koji/brew.

@rhatdan
Copy link
Member

rhatdan commented Jan 2, 2020

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: NevilleC, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 2, 2020
@rhatdan
Copy link
Member

rhatdan commented Jan 2, 2020

@NevilleC could you make sure that the rpm spec file still works.

@rhatdan
Copy link
Member

rhatdan commented Jan 4, 2020

I think you should enable it in the build script.

Changes I am making:

1. The target `.gopathok` was listed in `.PHONY` which
looks wrong as it regenerates `.gopathok` every time we
 re-run it, which was a part of the issue. I removed it
 to avoid that. If `.gopathok` is present', makefile
should not need to rerun it.

2. Ensure the binaries are created only if they don't
exist by adding `bin/podman`
and `bin/podman-remote`.

3. Add a `SOURCES = $(shell find . -name "*.go")` and put
it as a dependency of the podman binaries target. It allows us
to re-generate the binaries only when there is a change in the
source files. The downside is it increases the running time of
the command that generates them (20 seconds on my virtual machine
running Centos 7). If this is a problem, we could introduce a
hidden file that would list all the files to track, that
would need to be updated only when a dev is introducing new files.

4. Fixed the make package-install as it does not work with yum.
I updated the build_rpm.sh to ensure it works on centos 7
and centos 8 with no pre-required installation.

Closes containers#4367

Signed-off-by: Neville Cain <[email protected]>
@NevilleC
Copy link
Collaborator Author

NevilleC commented Jan 4, 2020

/retest

@NevilleC NevilleC changed the title [Issue #4367] Generate binaries only if there are changes in src code. Generate binaries only if there are changes in src code. Jan 5, 2020
@NevilleC
Copy link
Collaborator Author

NevilleC commented Jan 5, 2020

it seems to be a temp issue in ci/prow/images

Error: Error downloading packages:
  Curl error (6): Couldn't resolve host name for https://mirrors.fedoraproject.org/metalink?repo=updates-released-f29&arch=x86_64 [Could not resolve host: mirrors.fedoraproject.org]
error: build error: running 'dnf install -y rpms/x86_64/*' failed with exit code 1

I will retry re-test one last time.

/retest

@edsantiago
Copy link
Member

I'm sorry for causing so much churn and confusion: my comments referred to the real spec build, as in the one in fedpkg, not the fake spec in this repo. Since I still can't find anything relevant in the history, I vote to just approve this and then see if koji fails.

/retest

@edsantiago
Copy link
Member

@rhatdan @cevich what is rdoproject.org/github-check?

@mheon
Copy link
Member

mheon commented Jan 6, 2020

Openstack TripleO gating tests

@mheon
Copy link
Member

mheon commented Jan 6, 2020

I think we're not expecting that to go green yet - @ssbarnea is that right?

@mheon
Copy link
Member

mheon commented Jan 6, 2020

For the record, I think we're good to merge once that's clarified.

@edsantiago
Copy link
Member

Openstack TripleO gating tests

I phrased my question improperly - I should've asked, why are they failing, why is there no actual log (the link is useless), how does a regular user restart or waive that, and where is there documentation on this newly-randomly-added CI test?

@cevich
Copy link
Member

cevich commented Jan 6, 2020

Link is working for me now but page is tl;dr. This SNAFU reads like growing-pains to me, probably just need to wait for clarification. It would be up to the (other) CI system to set the status as ignored instead of failed. There's nothing in github directly that can set a non-native status to other that what was reported. The only option from a github POV is 'X is required for merge' (which is not currently set).

@edsantiago
Copy link
Member

Link is working for me now

If you look closely you'll see that link is of the form .../long-identifier; whereas the page that loads is merely /status with no identifier. That suggests to me that the actual results page is 404, and the rdoproject site is redirecting that to a generic status page.

@cevich
Copy link
Member

cevich commented Jan 6, 2020

a generic status page

OIC, yes. it could also be that you need to be logged in to that site and/or have permissions see the actual status.

FWIW, this behavior is happening on all/many other PRs as well.

@mheon
Copy link
Member

mheon commented Jan 6, 2020

Given this, I'm going to say we see if it merges even with the check red.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 6, 2020
@cevich
Copy link
Member

cevich commented Jan 6, 2020

all/many other PRs as well.

Meaning, the check fails. Looking now, the status-page thing does works on other PRs. Still, seems like growing-pains related issues overall.

@cevich
Copy link
Member

cevich commented Jan 6, 2020

merges even with the check red.

Yes, should be able to. AFAIK that check isn't marked as required in the repo. config.

@openshift-merge-robot openshift-merge-robot merged commit 2d8f1c8 into containers:master Jan 6, 2020
@NevilleC NevilleC deleted the nc-issue4367 branch January 6, 2020 19:19
@NevilleC
Copy link
Collaborator Author

NevilleC commented Jan 6, 2020

thanks all for help and feedback!

@edsantiago
Copy link
Member

Followup: rawhide build is fine, and so are gating tests. I'm not sure what I was (mis)remembering, but this clearly seems to be working. Thanks for your PR.

@NevilleC
Copy link
Collaborator Author

NevilleC commented Jan 6, 2020

great, thanks for the testing @edsantiago

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make install should not make binaries if already exist
7 participants