search

cornelisnetworks / opa-ff

Other
11 stars 20 forks source link

Add shebang for exp files #1

Open nirmoy opened 8 years ago

sjb017 commented 8 years ago

Hello Nirmoy,

Thank you for this pull request. On the surface it makes sense to me and it appears you address every .exp file in the repository. I’d like to put it through an internal review with our engineering team just to be safe. Once we review this we will let you know our response.

Thank you, Scott Breyer Maintainer, 01org/opa-ff

From: Nirmoy Das [mailto:notifications@github.com] Sent: Friday, April 15, 2016 6:58 AM To: 01org/opa-ff Subject: [01org/opa-ff] Add shebang for exp files (#1)

You can view, comment on, or merge this pull request online at:

https://github.com/01org/opa-ff/pull/1

Commit Summary

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHubhttps://github.com/01org/opa-ff/pull/1

sjb017 commented 8 years ago

Hello Nirmoy,

We are wondering what problem you are trying to address. Did you experience a particular issue? Our product does not invoke these scripts directly or otherwise use them as standalone; rather, they are invoked using the Tcl mechanism via our tcl_proc file. The method already handles the invocation of expect.

Thank you, Scott Breyer Maintainer, 01org/opa-ff

From: Breyer, Scott J Sent: Friday, April 15, 2016 12:53 PM To: '01org/opa-ff' Subject: RE: [01org/opa-ff] Add shebang for exp files (#1)

Hello Nirmoy,

Thank you for this pull request. On the surface it makes sense to me and it appears you address every .exp file in the repository. I’d like to put it through an internal review with our engineering team just to be safe. Once we review this we will let you know our response.

Thank you, Scott Breyer Maintainer, 01org/opa-ff

From: Nirmoy Das [mailto:notifications@github.com] Sent: Friday, April 15, 2016 6:58 AM To: 01org/opa-ff Subject: [01org/opa-ff] Add shebang for exp files (#1)

You can view, comment on, or merge this pull request online at:

https://github.com/01org/opa-ff/pull/1

Commit Summary

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHubhttps://github.com/01org/opa-ff/pull/1

nirmoy commented 8 years ago

Hi Scott,

I am trying RPM packaging of opa-ff and our build system throws warning while packaging files without shebang. The commit is not fixing any problem within opa-ff but I think it make sense to have those shebang.

sjb017 commented 8 years ago

Hello Nimroy,

We would like to propose an alternate solution, that these .exp files change the file mode permissions so that they are not executable. To repeat, we have no intention of these files being run as standalone, and might this remove the need for the shebang changes?

Thank you, Scott

From: Nirmoy Das [mailto:notifications@github.com] Sent: Monday, April 18, 2016 11:57 AM To: 01org/opa-ff opa-ff@noreply.github.com Cc: Breyer, Scott J scott.j.breyer@intel.com Subject: Re: [01org/opa-ff] Add shebang for exp files (#1)

Hi Scott,

I am trying RPM packaging of opa-ff and our build system throws warning while packaging files without shebang. The commit is not fixing any problem within opa-ff but I think it make sense to have those shebang.

— You are receiving this because you commented. Reply to this email directly or view it on GitHubhttps://github.com/01org/opa-ff/pull/1#issuecomment-211444090

ghost commented 8 years ago

Please replace '#!/usr/bin/env expect' with '#!/usr/bin/expect -f'. The sub package which includes the exp scripts will require '/usr/bin/expect'. So, when you install opa packages with dnf or yum, the package which provides '/usr/bin/expect' will be installed as required.