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

(FACT-1321) Install facter batch files on windows #1263

Conversation

melissa
Copy link

@melissa melissa commented Jan 26, 2016

As we switch to building puppet-agent for windows with vanagon, we need
access to resources that currently live in puppet_for_the_win. Though
these resources depend on resources that are made available in
puppet-agent, it makes sense to store these in the project specific
repo.

This takes advantage of environment.bat in puppet-agent installed with puppetlabs/puppet-agent#512

@melissa melissa changed the title (FACT-1321) Install facter batch files on windows [WIP](FACT-1321) Install facter batch files on windows Jan 26, 2016
) else (
SET PATH=%~dp0;%PATH%
)
elevate.exe "%~dp0facter_interactive.bat"
Copy link
Author

Choose a reason for hiding this comment

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

facter.bat and facter_interactive.bat work fine, but I'm having trouble with run_facter_interactive.bat. It seems like the call to elevate.exe isn't right. Any thoughts?

cc/ @joshcooper @MikaelSmith

Copy link
Author

Choose a reason for hiding this comment

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

Also, puppetlabs/puppet-agent#515 will install elevate.exe and elevate.exe.config

Choose a reason for hiding this comment

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

It's just Cygwin.

Copy link
Author

Choose a reason for hiding this comment

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

This wasn't working in cygwin. Running from not-cygwin worked as expected

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know the path to elevate.exe relative to the directory where run_facter_interactive.bat is installed?

Copy link
Author

Choose a reason for hiding this comment

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

if it's installed as a part of puppet-agent, it should be in the same directory

@melissa melissa force-pushed the ticket/master/FACT-1321-facter-batch-files branch from 7f728df to 8fe9caf Compare January 26, 2016 18:11
@melissa melissa changed the title [WIP](FACT-1321) Install facter batch files on windows (FACT-1321) Install facter batch files on windows Jan 27, 2016
@melissa melissa force-pushed the ticket/master/FACT-1321-facter-batch-files branch from 8fe9caf to 2b6202d Compare January 27, 2016 00:31
@shrug
Copy link

shrug commented Jan 27, 2016

👍

@melissa
Copy link
Author

melissa commented Jan 28, 2016

cc/ @MikaelSmith @kylog @joshcooper could one of you review this?

@@ -0,0 +1,8 @@
@echo off
SETLOCAL
if exist "%~dp0environment.bat" (
Copy link
Contributor

Choose a reason for hiding this comment

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

When will environment.bat not exist?

Copy link
Author

Choose a reason for hiding this comment

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

See comments on puppetlabs/hiera#340

The interesting part about facter is that these files aren't installed by the system, they have to be purposefully copied over. I figured that since they're in the public facter repo, theoretically they should be able to be used on their own. So, if someone just installs this on their system without environment.bat, it should still work. The other batch scripts assume ruby is in the working directory, so I kept that assumption here if environment.bat doesn't exist.

@MikaelSmith
Copy link

These look like they need to be installed. We could make them part of the CMake install task on Windows, something like what we do for man pages at https://github.com/puppetlabs/facter/blob/master/CMakeLists.txt#L152.

@melissa
Copy link
Author

melissa commented Feb 2, 2016

@MikaelSmith yeah, that seems totally legit. I wasn't sure how to do that. Thanks for pointing me to that example!

@melissa melissa force-pushed the ticket/master/FACT-1321-facter-batch-files branch from 2b6202d to a579af2 Compare February 3, 2016 01:27
@melissa
Copy link
Author

melissa commented Feb 3, 2016

It installs the files as expected on windows

C:\ProgramData\chocolatey\lib\cmake\content\cmake-3.2.2-win32-x86\bin\cmake.exe -P cmake_install.cmake
-- Install configuration: "Release"
-- Installing: C:/Program Files/Puppet Labs/puppet/share/man/man8/facter.8
-- Installing: C:/Program Files/Puppet Labs/puppet/bin/facter.bat
-- Installing: C:/Program Files/Puppet Labs/puppet/bin/facter_interactive.bat
-- Installing: C:/Program Files/Puppet Labs/puppet/bin/run_facter_interactive.bat
-- Installing: C:/Program Files/Puppet Labs/puppet/lib/libfacter.so.a
-- Installing: C:/Program Files/Puppet Labs/puppet/bin/libfacter.so
-- Installing: C:/Program Files/Puppet Labs/puppet/include/facter

and not on linux

/opt/pl-build-tools/bin/cmake -P cmake_install.cmake
-- Install configuration: "Release"
-- Installing: /opt/puppetlabs/puppet/share/man/man8/facter.8
-- Installing: /opt/puppetlabs/puppet/lib/libfacter.so.3.2.0
-- Installing: /opt/puppetlabs/puppet/lib/libfacter.so
-- Set runtime path of "/opt/puppetlabs/puppet/lib/libfacter.so.3.2.0" to "/opt/puppetlabs/puppet/lib"
-- Installing: /opt/puppetlabs/puppet/include/facter
-- Installing: /opt/puppetlabs/puppet/include/facter/facts
-- Installing: /opt/puppetlabs/puppet/include/facter/facts/external

cc/ @MikaelSmith

@melissa
Copy link
Author

melissa commented Feb 3, 2016

A change to puppet-agent that installed these new files before the cmake change was merged, and now needs to be reverted. puppetlabs/puppet-agent#526

@MikaelSmith
Copy link

Fine with this; I think it'd be cleaner to only have the batch files in "C:/Program Files/Puppet Labs/bin" and point to puppet/bin/facter.exe, but we can leave that for later. I'm not sure how you want to deal with that.

Making it happen could be done via configure_file to generate facter.bat from a template, and add an option to specify the install location for .bat files.

if exist "%~dp0environment.bat" (
call "%~dp0environment.bat" %0 %*
) else (
SET PATH=%~dp0;%PATH%

Choose a reason for hiding this comment

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

I think this needs to be quoted. When I try it out and %~dp0 is C:\Program Files\FACTER\bin I get the error

\FACTER\bin\ was unexpected at this time.

Choose a reason for hiding this comment

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

I was almost right, using SET PATH="%~dp0;%PATH%" creates an invalid PATH. It needs to be SET "PATH=%~dp0;%PATH%".

@melissa melissa force-pushed the ticket/master/FACT-1321-facter-batch-files branch from a579af2 to b90fd08 Compare February 3, 2016 22:20
@MikaelSmith
Copy link

Facter.bat in a stand-alone install doesn't seem to find ruby.exe. Not sure why. Investigating.

As we switch to building puppet-agent for windows with vanagon, we need
access to resources that currently live in puppet_for_the_win. Though
these resources depend on resources that are made available in
puppet-agent, it makes sense to store these in the project specific
repo.
@melissa melissa force-pushed the ticket/master/FACT-1321-facter-batch-files branch from b90fd08 to a1b734c Compare February 3, 2016 23:38
@MikaelSmith
Copy link

Looks good in stand-alone install and a puppet-agent build. 👍

MikaelSmith pushed a commit that referenced this pull request Feb 4, 2016
…batch-files

(FACT-1321) Install facter batch files on windows
@MikaelSmith MikaelSmith merged commit 1bd2ea3 into puppetlabs:master Feb 4, 2016
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.

4 participants