Conversation
|
Why not make it into a variant? The default could be up for debate (with a slight preference to "as configure without arguments would build it" from me, but idc) |
|
On Thu, May 17, 2018 at 7:20 AM, healther ***@***.***> wrote:
Why not make it into a variant?
Because someone would have to actually want it, know how to use it, and
verify that it works when turned on. When / if someone wants CSharp, they
should make it a variant.
The default could be up for debate (with a slight preference to "as
configure without arguments would build it" from me, but idc)
We've been through this with other packages... in general, it's best to
turn off all language bindings by default, if those bindings involve extra
dependencies.
|
|
I think I agree with @citibeth here, I don't like the idea of Spack packages linking to system libraries. If someone needs C# support, they'll need to add a C# package to Spack first. |
| '--{0}-python'.format('enable' if '+python' in spec else 'disable') | ||
| '--{0}-cxx'.format(E if '+cxx' in spec else D), | ||
| '--{0}-java'.format(E if '+java' in spec else D), | ||
| '--{0}-python'.format(E if '+python' in spec else D), |
There was a problem hiding this comment.
Aside from the --disable-csharp change, I would leave the rest of this package the same. PEP 8 says not to use capital letters in variable names. Also, single character variables names aren't very descriptive.
There was a problem hiding this comment.
I would leave the rest of this package the same.
Unfortunately I can't. Adding a comma to the '+python' line caused it to fail flake8 by going 1 character over the 79-char limit. The changes above are a solution to that problem.
PEP 8 says not to use capital letters in variable names.
PEP8 also says:
Constants are... written in all capital letters with underscores separating words. Examples include MAX_OVERFLOW and TOTAL.
Also, single character variables names aren't very descriptive.
The names are defined, then used immediately over the next 4 lines. Should not be a problem. And if they were longer, it would defeat the purpose.
If there's another solution to the original problem that you like better, please suggest it.
There was a problem hiding this comment.
A few Two things:
- Autotools packages have access to the
enable_or_disablefunction which should do this automatically for you - More generally, for flake, you can split after the
format:
return [
'--{0}-cxx'.format(E if '+cxx' in spec else D),
'--{0}-java'.format(E if '+java' in spec else D),
'--{0}-python'.format(
E if '+python' in spec else D),
'--disable-csharp'
There was a problem hiding this comment.
Move --disable-csharp to the beginning so python is still last?
|
The question is, what is the preferred approach? I would have been happy
to just leave the whole thing alone.
…On Thu, May 17, 2018 at 10:04 PM, scheibelp ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In var/spack/repos/builtin/packages/antlr/package.py:
> return [
- '--{0}-cxx'.format('enable' if '+cxx' in spec else 'disable'),
- '--{0}-java'.format('enable' if '+java' in spec else 'disable'),
- '--{0}-python'.format('enable' if '+python' in spec else 'disable')
+ '--{0}-cxx'.format(E if '+cxx' in spec else D),
+ '--{0}-java'.format(E if '+java' in spec else D),
+ '--{0}-python'.format(E if '+python' in spec else D),
A few things:
- Autotools packages have access to the enable_or_disable function
which should do this automatically for you
- More generally, for flake, you can split after the format:
return [
'--{0}-cxx'.format(E if '+cxx' in spec else D),
'--{0}-java'.format(E if '+java' in spec else D),
'--{0}-python'.format(
E if '+python' in spec else D),
'--disable-csharp'
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8157 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AB1cd7PJJ9AdpW77Jh9zhp2ZCOsZ-Tycks5tziwvgaJpZM4UBpB5>
.
|
As for that I don't think you can leave it alone (at least the The first two lines would be excluded from the diff. That's my preference. I'd also be fine with the use of the |
4 participants
On my SUSE11 system, ANLTR build of the CSharp bindings was failing due to a lack of system-installed CSharp on the system. Unless we add csharp support to Spack, we should disable the ANTLR CSharp bindings.
Note the available options for ANTLR's
./configurescript. This stuff isn't really documented anywhere else: