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

add shell specific packages #158

Merged
merged 1 commit into from
Apr 25, 2018
Merged

add shell specific packages #158

merged 1 commit into from
Apr 25, 2018

Conversation

dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Apr 24, 2018

Fixes #157. Connect to #157.

@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Apr 24, 2018
@dirk-thomas dirk-thomas self-assigned this Apr 24, 2018
@dirk-thomas dirk-thomas force-pushed the install_shell_packages branch from edb8cdf to 0242bc8 Compare April 24, 2018 02:10
@dirk-thomas
Copy link
Member Author

dirk-thomas commented Apr 24, 2018

  • Linux Build Status
  • macOS Build Status
  • Windows Build Status

@dirk-thomas dirk-thomas requested a review from dhood April 24, 2018 02:27
dhood
dhood previously approved these changes Apr 24, 2018
Copy link
Member

@dhood dhood left a comment

Choose a reason for hiding this comment

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

Is the only way to get bash support by adding argcomplete? I would have thought that these would be separate notions (and that I could get prefix.bash files generated even if I don't want to/can't install argcomplete)

Either way, if it fixes the packaging, then +1.

@dhood
Copy link
Member

dhood commented Apr 24, 2018

I might be calling it incorrectly, but it doesn't seem like the powershell script works even on the windows build machines:

<powershell terminal>
PS C:\Users\osrf\Downloads\ros2-windows-colcon\ros2-windows> .\prefix.ps1
At C:\Users\osrf\Downloads\ros2-windows-colcon\ros2-windows\prefix.ps1:16 char:66
+     echo "not found: '$_colcon_prefix_source_powershell_script'" 1>&2
+                                                                  ~~~~
The '1>&2' operator is reserved for future use.
    + CategoryInfo          : ParserError: (:) [], ParseException
    + FullyQualifiedErrorId : RedirectionNotSupported

Are you testing the result of the windows packages somehow?

@dhood
Copy link
Member

dhood commented Apr 24, 2018

edit: the machine I tested on (windshield) has colcon-powershell==0.0.0 while the machine the archive was built on (icecube) had 0.1.0, which might be related if there are runtime dependency minimums

@dirk-thomas
Copy link
Member Author

Is the only way to get bash support by adding argcomplete? I would have thought that these would be separate notions (and that I could get prefix.bash files generated even if I don't want to/can't install argcomplete)

These two feature (bash and argcomplete) come in two separate packages. colcon-bash provides the bash scripts, colcon-argcomplete provides completion for bash (and zsh). The first is usable without the second. I just thought that installing the second makes sense two. If the users have argcomplete installed they will also get completion functionality (if they don't it is just doing nothing for them).

@dirk-thomas
Copy link
Member Author

Are you testing the result of the windows packages somehow?

We are not using the PowerShell scripts yet. We are still relying on Batch. It has certainly worked in the past. Maybe it is a regression of colcon/colcon-powershell#2.

@dirk-thomas
Copy link
Member Author

The '1>&2' operator is reserved for future use.

Addressed by colcon/colcon-powershell#3.

@dhood dhood dismissed their stale review April 24, 2018 18:44

new understanding

@dhood
Copy link
Member

dhood commented Apr 24, 2018

OK I have a better understanding now of the impact of the dependencies.

These two feature (bash and argcomplete) come in two separate packages. colcon-bash provides the bash scripts, colcon-argcomplete provides completion for bash (and zsh). The first is usable without the second. I just thought that installing the second makes sense two. If the users have argcomplete installed they will also get completion functionality (if they don't it is just doing nothing for them).

I didn't follow the reasoning for installing optional dependencies on the buildfarm machines so I assumed it was required. We were trying to keep the dependencies minimal, so if argcomplete isn't required, we should remove it from this PR then, right? Or is it added so that the archives have argcomplete support or something? (in the past that was done based on user-installed dependencies not build-machine deps)

We are not using the PowerShell scripts yet. We are still relying on Batch. It has certainly worked in the past. Maybe it is a regression of colcon/colcon-powershell#2.

Given that powershell is not supported yet (e.g. we don't have matching env hooks for powershell) I think it's misleading to provide the powershell scripts in the archives, and think the colcon-powershell package should be removed from this PR and from the machines that have had it installed, so that the archives don't generate a prefix.ps1.

@dirk-thomas dirk-thomas force-pushed the install_shell_packages branch from 0242bc8 to 14f1ef4 Compare April 24, 2018 19:00
@dirk-thomas
Copy link
Member Author

I removed the argcomplete specific packages.

Given that powershell is not supported yet (e.g. we don't have matching env hooks for powershell) I think it's misleading to provide the powershell scripts in the archives, and think the colcon-powershell package should be removed from this PR and from the machines that have had it installed, so that the archives don't generate a prefix.ps1.

The fact that some packages don't have an environment hook atm is imo not a reason to remove it. We currently also don't recommend using the .ps1 file anywhere so it is unlikely that users will just use it and expect it to work.

I think it should stay in order to actually be able to test and use it. And potentially add the missing environment hooks where necessary.

@dhood
Copy link
Member

dhood commented Apr 24, 2018

I think it should stay in order to actually be able to test and use it. And potentially add the missing environment hooks where necessary.

I agree that this could be useful one day, but I don't agree with this statement:

We currently also don't recommend using the .ps1 file anywhere so it is unlikely that users will just use it and expect it to work.

Users will notice the file, just as I did, and try to use it, whether it is recommended or not. If it's shipped in our archives then yes, as a user, I would expect it to work. Since we ship archives with opensplice and I am of the understanding that opensplice support will not be available for powershell, having the system fail on a user in a strange way IMO is not an acceptable trade-off so that we can hypothetically test powershell at some time in the future.

If there isn't full support for powershell with the packages that are included in the archive, then the archive should not include prefix.sh1 in my opinion.

Easiest way to achieve that is to not install the dependency on the machines. If we think that's unavoidable then perhaps the packaging job should remove the file manually.

I don't believe this applies to zsh for example, because it wraps the sh env hooks from my understanding. If this is not correct, then we should remove the zsh file too is support is not complete.

@dirk-thomas
Copy link
Member Author

we can hypothetically test powershell at some time in the future.

We are talking about an imminent need to use PowerShell because of the limitations of the cmd shell. So this is not some distant idea but something we try to do in the coming days.

@dhood
Copy link
Member

dhood commented Apr 24, 2018

OK, sure, if we plan on adding full powershell support soon then we can leave it as is. If the next release rolls around and it's not complete, I suggest we remove the .ps1 file.

@dirk-thomas dirk-thomas merged commit 1a6d2c4 into master Apr 25, 2018
@dirk-thomas dirk-thomas deleted the install_shell_packages branch April 25, 2018 01:39
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Apr 25, 2018
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.

2 participants