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

Build of Free LAStools RPM from RapidLasso GmBH #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

johnpinto1
Copy link
Contributor

@@ -0,0 +1,27 @@
FROM centos:7

ENV version 170414
Copy link
Member

Choose a reason for hiding this comment

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

This variable isn't used in the file.

@@ -0,0 +1,16 @@
#!/bin/bash

version=170414
Copy link
Member

Choose a reason for hiding this comment

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

Could you use 2017 rather than the shorthand of 17 please.

Copy link
Contributor Author

@johnpinto1 johnpinto1 Apr 17, 2017

Choose a reason for hiding this comment

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

Updated code as requested.
Note: 170414 used by rpm installed on dm-data,
lastools-170414-1.el7.x86_64.rpm.

Also if you run version for installed tools you get the following for all of then:

bin ⮀ laszip -version
LAStools (by martin@rapidlasso.com) version 170414
bin ⮀ las2txt -version
LAStools (by martin@rapidlasso.com) version 170414

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think changing the version is a good idea, the version currently used, is what you get when you run add the --version option to any of the commands. Changing it is a bad idea, who cares if it represents a year or not, more important, to me at least, is that the versions match, changing it, even on the file name is not a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I didn't realise thats the version that the binary returned. In which case I agree. I thought it was just chosen by John as the date of the last changes in the CHANGES file.

Copy link
Contributor

@marksmall marksmall Apr 18, 2017

Choose a reason for hiding this comment

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

At the moment getting the version is a manual step. In a year's time is anyone going to remember how we got this version number (minus any comments of course). Better would be to, in the docker script or wherever unzips the binaries:

  1. unzip the binary
  2. Run laszip --version (capturing the version number)

This has the benefit of being self-documenting, we know where the version comes from and keeps it in sync

Copy link
Member

Choose a reason for hiding this comment

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

Sounds fine if that will work. Otherwise a comment is probably sufficient.

…0414. Remove unused version variable in Dockerfile.
@johnpinto1
Copy link
Contributor Author

Updated code to capture version in fpm.sh by running "laszip -version".

we now capture the version by running "laszip -version".
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.

3 participants