-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
IGNITE-7107 Apache Ignite RPM packages #3171
Conversation
vveider
commented
Dec 7, 2017
- added spec for RPM build
- added instructions to DEVNOTES.txt
DEVNOTES.txt
Outdated
@@ -4,81 +4,126 @@ Ignite Fabric Maven Build Instructions | |||
|
|||
2) Compile and install: | |||
|
|||
mvn clean install -Pall-java,all-scala,licenses -DskipTests | |||
mvn clean install -Pall-java,all-scala,licenses -DskipTests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the file reformatted using tabs? Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My fault - got used to editing such files in default vim. Fixed and updated vim settings to indent with spaces.
# | ||
|
||
%install | ||
cd %{name}-* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem to work:
+ cd 'apache-ignite-*'
/var/tmp/rpm-tmp.KwOoyl: line 34: cd: apache-ignite-*: No such file or directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange error - shell expansion is not working. Anyway, replaced with more universal variant.
<bean class="org.apache.ignite.spi.discovery.tcp.TcpDiscoverySpi"> | ||
<property name="ipFinder"> | ||
<bean class="org.apache.ignite.spi.discovery.tcp.ipfinder.multicast.TcpDiscoveryMulticastIpFinder"> | ||
<property name="multicastGroup" value="228.10.10.157"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should definitely not have configurations inline in build scripts!
Nobody will bother to update them, and there's duplication of effort, and of course this particular IP should not be here.
We should discuss whether default configuration should be multicast or VM on localhost.
Anyway, we should just copy default config from Ignite distribution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted all cat<<EOF
sections into separate files.
[Install] | ||
WantedBy=multi-user.target | ||
EOF | ||
cat <<EOF > %{buildroot}%{_datadir}/%{name}/bin/service.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this file should not be here inline, instead in packaging/ or bin/ in distribution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted all cat<<EOF
into separate files.
eb11004
to
67d20c0
Compare
* added spec for RPM build * added instructions to DEVNOTES.txt
* purged 'fabric' mentions from RPM package
* fixed review issues
6c4434e
to
bb6231f
Compare
* removed node.cfg and redesigned service to be able to run in multiple instances * removed replacement for default-config.xml from package * fixed DEVNOTES.txt
* prepared ignitesqlline | ignitevisorcmd future usage from native bin
* moved sqlline back to ${IGNITE_HOME}/bin
* updated DEVNOTES.txt