Setting up rpm spec for odo for rpm build #2107
Conversation
scripts/rpm-prepare.sh
Outdated
| echo "Generating spec file $SPEC_DIR/atomic-openshift-odo-rpm.spec" | ||
| envsubst <rpms/atomic-openshift-odo-rpm.spec > $SPEC_DIR/atomic-openshift-odo-rpm.spec | ||
|
|
||
| echo "Generating taball $SOURCES_DIR/$NAME.tar.gz" |
| export ODO_RELEASE=${ODO_RELEASE:=1} | ||
| export GIT_COMMIT=${GIT_COMMIT:=`git rev-parse --short HEAD 2>/dev/null`} | ||
| export ODO_RPM_VERSION=${ODO_VERSION//-} | ||
|
|
There was a problem hiding this comment.
It would be nice to print the values of ODO_VERSION, ODO_RELEASE, GIT_COMMIT and ODO_RPM_VERSION here as verification that the correct one are used
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kadel 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 |
openshift-ci-robot
added
the
approved
|
/retest |
|
CI is flaky |
|
The $ ./scripts/rpm-local-build.sh
Copying content to local rpmbuild
'dist/rpmbuild/SOURCES/atomic-openshift-odo-1.0.0test-1.tar.gz' -> '/home/dshah/rpmbuild/SOURCES/'
cp: cannot create regular file '/home/dshah/rpmbuild/SOURCES/': No such file or directory
'dist/rpmbuild/SPECS/atomic-openshift-odo.spec' -> '/home/dshah/rpmbuild/SPECS/'
cp: cannot create regular file '/home/dshah/rpmbuild/SPECS/': No such file or directory
Building locally
error: failed to stat /home/dshah/rpmbuild/SPECS/atomic-openshift-odo.spec: No such file or directory
$ ls ~/rpmbuild
BUILD BUILDROOT RPMS SOURCES SPECS SRPMSAm I missing something that needs to be done to build rpm as a normal Linux user? |
| echo "Making release for $NAME, git commit $GIT_COMMIT" | ||
|
|
||
| echo "Cleaning up old content" | ||
| if [[ -d $DIST_DIR ]]; then |
There was a problem hiding this comment.
Since we're doing an rm -rf, why do we even need the if checks in these two cases?
There was a problem hiding this comment.
hmm makes sense as rm -rf never fails but still., it's better to check. file exists before any operation.
scripts/rpm-prepare.sh
Outdated
| pushd $NAME | ||
| # Remove bin if it exists, we dont need it in tarball | ||
| if [[ -f ./odo ]]; then | ||
| rm -rf ./odo |
There was a problem hiding this comment.
Again, why have an if when we're doing rm -rf?
rpms/atomic-openshift-odo.spec
Outdated
| #this is a template spec and actual spec will be generated | ||
| #debuginfo not supported with Go | ||
| %global debug_package %{nil} | ||
| %global package_name atomic-openshift-odo |
There was a problem hiding this comment.
I have not reviewed any spec files in the past. Heck, I don't even understand them so please pardon me if the query is silly/naive.
Does this mean that yum install atomic-openshift-odo will install the odo binary for the users? If yes, out of sheer curiosity, why do we have atomic in the name?
If no, what does this mean? 😄
There was a problem hiding this comment.
I have not reviewed any spec files in the past. Heck, I don't even understand them so please pardon me if the query is silly/naive.
Does this mean that
yum install atomic-openshift-odowill install theodobinary for the users? If yes, out of sheer curiosity, why do we haveatomicin the name?If no, what does this mean?
yes and no both. It is actually prefixed to package name
full name = package_name-odo_version-odo_release.dist.arch.rpm
the atomic-openshoft is rec prefix for all openshift packages
There was a problem hiding this comment.
Does this mean that yum install atomic-openshift-odo will install the odo binary for the users?
Yes.
Its upto us to decide what we want to name, the naming discussions are always ..
but we can align to what's done for oc, oc is provided by openshift-clients RPM, which is a RPM of openshift package (multiple RPMs, part of one package).
There was a problem hiding this comment.
Does this mean that yum install atomic-openshift-odo will install the odo binary for the users?
Yes.
Its upto us to decide what we want to name, the naming discussions are always ..
but we can align to what's done foroc,ocis provided byopenshift-clientsRPM, which is a RPM ofopenshiftpackage (multiple RPMs, part of one package).
Thanks @navidshaikh for the response. It helps. And I agree that naming discussions are always... I'm afraid I'm the one who's making it painful this time. 😂
@kadel @mohammedzee1000 do we really need atomic in the name of the rpm that provides odo binary? I feel that openshift-odo is good enough. I don't see why we want to connect this with Project Atomic which is, AFAIU, more about underlying container technologies.
There was a problem hiding this comment.
well @kadel i will leave final call to you. But i would require the decision at earliest as i will need to raise rcm ticket to set up a new brew package (removing old one in the process)
There was a problem hiding this comment.
It should not matter too much in my book as the rpm is for rcm satisfaction and the paper trail. We will be delivering it to other delivery mechanisms such as mirror / cdn / cli artifacts image and a few mad red hatters who might want the rhel rpm :P
Most devs will not be developing on rhel to care about the directly delivery of rpm
There was a problem hiding this comment.
If it is possible I would drop the atomic prefix.
I think this was happening because |
|
/retest |
rpms/atomic-openshift-odo.spec
Outdated
| install -d %{buildroot}%{_datadir}/%{name}/{linux,macos,windows} | ||
| install -p -m 755 dist/bin/linux-amd64/odo %{buildroot}%{_datadir}/%{name}/linux/odo-linux-amd64 | ||
| install -p -m 755 dist/bin/darwin-amd64/odo %{buildroot}%{_datadir}/%{name}/macos/odo-darwin-amd64 | ||
| install -p -m 755 dist/bin/windows-amd64/odo.exe %{buildroot}%{_datadir}/%{name}/windows/odo-windows-amd64.exe |
There was a problem hiding this comment.
For following 4 lines
install -d %{buildroot}%{_datadir}/%{name}/{linux,macos,windows}
install -p -m 755 dist/bin/linux-amd64/odo %{buildroot}%{_datadir}/%{name}/linux/odo-linux-amd64
install -p -m 755 dist/bin/darwin-amd64/odo %{buildroot}%{_datadir}/%{name}/macos/odo-darwin-amd64
install -p -m 755 dist/bin/windows-amd64/odo.exe %{buildroot}%{_datadir}/%{name}/windows/odo-windows-amd64.exe
I'd use the name %{name}-redistributable to align with -redistributable RPM.
If we change this as suggested, respective change needs to be done in %files section too.
There was a problem hiding this comment.
ok will take care of that, thanks
scripts/rpm-prepare.sh
Outdated
| rm -rf ./odo | ||
| fi | ||
| # Replace version info | ||
| sed -i "s/v[0-9]*.[0-9]*.[0-9]*-\w*/"${ODO_VERSION}"/g" pkg/odo/cli/version/version.go |
There was a problem hiding this comment.
this should not be edited here.
The version change should be committed into the git repo before the build is triggered
There was a problem hiding this comment.
instead it should be inserted via ldflags
There was a problem hiding this comment.
+1 In fact ldflags is the best route forward.
There was a problem hiding this comment.
Ok removed we should document this fact though, that at the very least the file be edited before running script
There was a problem hiding this comment.
if it has to edit manually, its better done in this script (avoids building without updating the version) and update the script later when ldflags are in place
There was a problem hiding this comment.
The main thing is that the update to the version.go should be committed to the git to have a record of it.
There was a problem hiding this comment.
It might be good to have a version file then with all necessary information. Any changes get committed to version file and then scripts take care of replacement be it via sed or ldflags or in the RPM spec file. That way we have commits on record at same time taking advantage of automation so nothing is missed, wdyt
mohammedzee1000
force-pushed
the
spec
branch
2 times, most recently
from
e108cc5
to
ddbb8e4
Compare
|
Seems to be some issue related to accessing github /retest |
|
@mohammedzee1000 can you please drop the cc @kadel |
|
RCM ticket raised to update brew target |
Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>
Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>
mohammedzee1000
force-pushed
the
spec
branch
from
ddbb8e4
to
d8abbbc
Compare
|
RCM ticket is being serviced and should close soon. It should be safe to move forward |
openshift-ci-robot
added
the
lgtm
rm3l
added
the
estimated-size/L (20-40)
What this is
This add rpm spec to odo repository.
As per @navidshaikh from knative, it will be easier if we first have rpms as ART already has a ready-made script for extracting odo binaries from rpms, so moving to add spec. Also having rpm makes it easy to have a proper paper trail
Note it is assumed rpmbuild is installed, on Fedora do
dnf install rpm-buildOutput