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

rpm generation process update #1980

Merged
merged 1 commit into from May 9, 2018

Conversation

szemere
Copy link
Collaborator

@szemere szemere commented Apr 11, 2018

RPM generation process update from @czanik.
This is the first step in a long process, the final goal is to harmonize Peter's work and /dbld

  • remove syslog-ng-add-contextual-data.pc from file list
  • update to 3.14.1
  • re-enable amqp & mongodb on RHEL only
  • keep tcp wrappers support only on RHEL

Fixes: #1857

@kira-syslogng
Copy link
Contributor

success

Copy link
Collaborator

@bazsi bazsi left a comment

Choose a reason for hiding this comment

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

It looks good to me.

@@ -2,3 +2,5 @@
libdbi
libdbi-drivers
riemann-c-client-devel
syslog-ng-java-deps
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does this package come from?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's not the githead branch in your repo, but version "3.11":
https://github.com/balabit/syslog-ng/blob/4ab34df81a7f97740e60aa5b0748c54b6518338b/dbld/images/helpers/functions.sh#L14-L17
This is used during docker image creation.

# CzP's spec file expect it in the pwd
# where the build was initiated
cp packaging/rhel/syslog-ng.* /build

Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding is that they need to be in the rpmbuild source directory. Isn't that the case?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is looking for the files where rpmbuild is initiated. In our case it is '''/build'''

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was experimenting with it as well.
rpmbuild is using the dist tarball, isn't it?
Why don't we include it our Makefile instead of this copy? I have tried it out and it works, it would be similar to what our debian packaging does.

# CzP's spec file expect it in the pwd
# where the build was initiated
cp packaging/rhel/syslog-ng.* /build

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was experimenting with it as well.
rpmbuild is using the dist tarball, isn't it?
Why don't we include it our Makefile instead of this copy? I have tried it out and it works, it would be similar to what our debian packaging does.

@@ -2,3 +2,5 @@
libdbi
libdbi-drivers
riemann-c-client-devel
syslog-ng-java-deps
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's not the githead branch in your repo, but version "3.11":
https://github.com/balabit/syslog-ng/blob/4ab34df81a7f97740e60aa5b0748c54b6518338b/dbld/images/helpers/functions.sh#L14-L17
This is used during docker image creation.

@gaborznagy
Copy link
Collaborator

This is a huge step to resolve #1857 👍

@MrAnno
Copy link
Collaborator

MrAnno commented Apr 13, 2018

Hi,

We've just merged a CI-related pull request into our master branch.
Since the current Travis CI checks in your PR are outdated, I'd like to ask you to rebase against master.

@gaborznagy gaborznagy changed the title rpm generation process update: [WIP] rpm generation process update Apr 16, 2018
@kira-syslogng
Copy link
Contributor

success

@kira-syslogng
Copy link
Contributor

success

@kira-syslogng kira-syslogng changed the title [WIP] rpm generation process update rpm generation process update Apr 18, 2018
@czanik
Copy link
Contributor

czanik commented Apr 19, 2018

Did a quick test in a completely clean environment. Running dbld/rules rpm-centos7 fails with dependency problems only if dbld/rules image-centos7 is not run first. Thanks to @szemere it now also works if it was invoked as root.

As discussed IRL with @gaborznagy , my githead repo should be used instead of 311 to make sure that the right version of dependencies are used.

@gaborznagy
Copy link
Collaborator

gaborznagy commented Apr 19, 2018

@czanik : by completely clean environment you mean you did not have any of the docker images prior testing, right?
We've rebuilt the centos7 docker images on dockerhub a couple of days ago, automatic docker pull should be working now.

About the changes:

  • I have tested rpm generating in dbld and it worked when I built the image prior to testing.
  • I have not tested it dbld with your githead repo so far.
  • I still need to find resolution for the problems I found when I included the packaging/rhel/ dir's content in the dist tarball:
  1. There is another spec in the repo, I have removed it from the dist tarball for testing, so there will be only the packaging/rhel spec file in the tarball.
  2. I tried to remove many unnecessary copying of the packaging dir (this is common in dbld).
  3. Your spec file expects files under the build/ dir which now has to be copied (this is what's in this PR), I try to change this to use files from the tarball instead.

@szemere
Copy link
Collaborator Author

szemere commented Apr 23, 2018

Running dbld/rules rpm-centos7 fails with dependency problems only if dbld/rules image-centos7 is not run first.

You are absolutely right. A little background information:
Because of a configuration mistake in Docker Hub, the latest TAG pointed to an outdated version of the image, from the time before #1876 . (Obviously with outdated dependencies.)
By running dbld/rules image-centos7 the image was (re)built locally and overrode whatever was downloaded from Docker Hub.
I corrected the Docker Hub config on April 17th, so the latest tag will represent the last commit from the master branch. By running docker pull balabit/syslog-ng-centos7 manually you can override your local version without the long build process.

Copy link
Collaborator

@furiel furiel left a comment

Choose a reason for hiding this comment

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

I reviewed and it looks good to me in general. If Gabors comment on wrong version: 3.11 is resolved, approve from my side.

@gaborznagy gaborznagy dismissed their stale review April 26, 2018 10:45

"dbld/rpm" will work with this patch and it's awaited. However it's not a sophisticated solution, it needs more work.

@gaborznagy
Copy link
Collaborator

This PR is awaited.
Let's test "githead" repo instead of "3.11" and if it works, merge the PR in.

The resolution of the many copying of the packaging dir in dbld will be resolved later.

@furiel furiel changed the title rpm generation process update [WIP] rpm generation process update May 7, 2018
  - remove syslog-ng-add-contextual-data.pc from file list
  - update to 3.15.1
  - re-enable amqp & mongodb on RHEL only
  - keep tcp wrappers support only on RHEL
  - updated epel: centos7->git_head
@szemere
Copy link
Collaborator Author

szemere commented May 8, 2018

Updated epel repo url to git head.
Version bumped the spec file.

@szemere szemere changed the title [WIP] rpm generation process update rpm generation process update May 8, 2018
@kira-syslogng
Copy link
Contributor

success

Copy link
Collaborator

@furiel furiel left a comment

Choose a reason for hiding this comment

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

I tried the patchset manually. I could build image-centos7 successfully. The rpm's are generated successfully. I could successfully install the generated rpm-s inside image-centos7, and syslog-ng started with an stdin source stdout destination, log are generated as expected.

All looks good to me! Thanks for putting this together.

@gaborznagy gaborznagy merged commit 3ae93e3 into syslog-ng:master May 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants