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

Makefile cleanup #91

Merged
merged 9 commits into from
May 19, 2017
Merged

Makefile cleanup #91

merged 9 commits into from
May 19, 2017

Conversation

nmeyerhans
Copy link
Contributor

This change updates several of the RPM package Makefile rules to have proper prerequisites such that they can run without having needed to explicitly run the prerequisite rule.

For example, the srpm rule automatically invokes the sources.tgz rule to generate the required sources.tgz file:

[nmeyerha@890b93b0265f amazon-ecs-init]$ git status
On branch makefile-cleanup
nothing to commit, working directory clean
[nmeyerha@890b93b0265f amazon-ecs-init]$ make srpm
./scripts/update-version.sh
++ dirname ./scripts/update-version.sh
+ CURRENTDIR=./scripts
++ cat ./scripts/../ecs-init/VERSION
+ version=1.14.1
++ git rev-parse --short HEAD
+ git_hash=464fde2
+ git_dirty=false
++ git status --porcelain
+ [[ '' != '' ]]
+ cat
./scripts/update-version.sh
++ dirname ./scripts/update-version.sh
+ CURRENTDIR=./scripts
++ cat ./scripts/../ecs-init/VERSION
+ version=1.14.1
++ git rev-parse --short HEAD
+ git_hash=464fde2
+ git_dirty=false
++ git status --porcelain
+ [[ '' != '' ]]
+ cat
cp packaging/amazon-linux-ami/ecs-init.spec ecs-init.spec
cp packaging/amazon-linux-ami/ecs.conf ecs.conf
tar -czf ./sources.tgz ecs-init scripts
test -e SOURCES || ln -s . SOURCES
rpmbuild --define "%_topdir /home/nmeyerha/go/src/github.com/aws/amazon-ecs-init" -bs ecs-init.spec
warning: bogus date in %changelog: Tue Nov 14 2016 Peng Yin <penyin@amazon.com> - 1.13.1-1
Wrote: /home/nmeyerha/go/src/github.com/aws/amazon-ecs-init/SRPMS/ecs-init-1.14.1-1.amzn1.src.rpm
find SRPMS/ -type f -exec cp {} . \;
touch .srpm-done

Additionally, the autogenerated ecs-init/version/version.go is removed from git and generated as needed by a make rule.

The clean rule is updated to correctly clean up artifacts. .gitignore is updated to ensure that generated or ephemeral files are not tracked.

Noah Meyerhans added 3 commits May 12, 2017 22:51
Fix the RPM generation rules to have proper prerequisites.
 * The 'srpm' and 'rpm' targets will both create the sources.tgz file if
   needed.
 * The 'rpm' rule will properly populate the rpmbuild SOURCES directory.
ecs-init/version/version.go is automatically generated, so it should be
tracked in git. This change removes it from git and adds its generation
as a prerequisite of make rules as appropriate.
Makefile Outdated
@@ -42,17 +45,28 @@ build-mock-images:
docker build -t "test.localhost/amazon/wants-update" -f "scripts/dockerfiles/wants-update.dockerfile" .
docker build -t "test.localhost/amazon/exit-success" -f "scripts/dockerfiles/exit-success.dockerfile" .

sources:
sources.tgz: ecs-init/version/version.go
./scripts/update-version.sh
Copy link
Member

Choose a reason for hiding this comment

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

This update-version.sh shouldn't be needed if you're generating it as a dependent target.

.gitignore Outdated
/SRPMS
/ecs-init.spec
/sources.tgz
ecs-init/version/version.go
Copy link
Member

Choose a reason for hiding this comment

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

I must have missed the discussion on moving away from committing the generated version.go, is there a good reason to not commit the file when it changes even if it is generated?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm strongly opposed to adding version.go to .gitignore. I understand that there are concerns with the file changing on every commit; I think a better option would be to remove the need to generate the file and instead use -ldflags to inject the values at link time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jahkeup The generated file contains the git sha. Committing it, by definition, renders it invalid.

Copy link
Member

Choose a reason for hiding this comment

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

One approach I've seen used for golang based CLI releases use a previous commit (tagged or otherwise) so that the dirty repository is only dirtied by the change to the version.go. However, the approach @samuelkarp mentions is my preference using something along the lines of https://github.com/cogolabs/lgrep/blob/c4efafa3ad84faa51df0bc48bc5c4e702c9b4b8f/Makefile#L4 or https://github.com/coreos/etcd/blob/31e3899663522556e06933297e12fd030efae488/build#L15 where the Version variables are overriden at build time. I would also rather not eliminate this file entirely.

Copy link
Member

@jahkeup jahkeup left a comment

Choose a reason for hiding this comment

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

🚢

Copy link
Contributor

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

👍

@nmeyerhans nmeyerhans merged commit 008838e into aws:dev May 19, 2017
@nmeyerhans nmeyerhans deleted the makefile-cleanup branch May 19, 2017 00:34
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.

None yet

4 participants