search

chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.8k stars 421 forks source link

Improve sys_getenv documentation #16705

Closed tmacd8 closed 3 years ago

tmacd8 commented 4 years ago

Summary of Problem

The online documentation for function sys_getenv could be improved. The prototype is:

procsys_getenv(name: c_string, ref string_out: c_string): c_int

yet the meaning of the return value is not clear. Some experimentation shows that if the value 1 is returned then the environment variable (i.e., name) is defined and if the value 0 is returned then the environment variable is not defined. The Standard C library defines getenv to return a pointer if defined and NULL if not defined. Making the return values (0 and 1) and their meaning (undefined and defined) explicit in the documentation would help. Or, perhaps, just saying that a return value of 0 means undefined and all other values mean defined.

mppf commented 4 years ago

Actually it is documented here:

https://github.com/chapel-lang/chapel/blob/36921709813e2c221505ad518b1014a8ed6a43a9/runtime/src/qio/sys.c#L481-L486

However that does not mean it couldn't/shouldn't be documented in Sys.chpl. But, we are working on doing module reviews to stabilize modules (so that they do not change in the future and break programs). While this routine might continue to exist in the C runtime, we will almost certainly need to make a more Chapel-y way to get an environment variable (say, one that returns a string).

In any case I think we would be happy to have a PR more or less the documentation to Sys.chpl for the near term. You're welcome to prepare such a thing and you can check the rendered docs with make docs.

bradcray commented 4 years ago

I disagree with the tone of Michael's previous comment, which seems to suggest that the issue as written is somehow invalid or not our problem. The documentation for the Sys module states that to learn how a routine sys_foo() works, one should read man foo, but for sys_getenv() the signature described in man getenv doesn't match the Chapel routine, with no additional indication of how to translate between them. I don't think it's reasonable to expect a user to find their way from our online documentation into the runtime source code that implements it to find a comment that's there. And given that Sys is a standard module, I think this is definitely a reasonable request that's on us by default.

That said, I completely agree with him that with the library review and stabilization effort we're undertaking (see slide 20 of these release notes), it is likely that these routines will be replaced with ones with a more Chapeltastic interface, so whether or not it's worth the immediate effort to fix this routine (given that there are probably other routines within the Sys module that would need similar documentation improvements) given that it's likely to change is a reasonable question.

tmacd8 commented 4 years ago

Making functions like getenv be more chapel-y seems like a good idea to me. I don't believe it's a high priority. I had to write a little testcase and discover for myself what the return values meant and it just felt like I should have been able to figure it out by reading the docs. Anyway, I defer to all of your collective best judgements on the best way to handle this. If you want me to mark this issue as 'resolved' then I'm happy to do so.

-- TMacD

On Mon, Nov 16, 2020 at 11:27 AM Brad Chamberlain notifications@github.com wrote:

I disagree with the tone of Michael's previous comment, which seems to suggest that the issue as written is somehow invalid or not our problem. The documentation for the Sys module https://chapel-lang.org/docs/master/modules/standard/Sys.html states that to learn how a routine sys_foo() works, one should read man foo, but for sys_getenv() the signature described in man getenv doesn't match the Chapel routine, with no additional indication of how to translate between them https://chapel-lang.org/docs/master/modules/standard/Sys.html#Sys.sys_getenv. I don't think it's reasonable to expect a user to find their way from our online documentation into the runtime source code that implements it to find a comment that's there. And given that Sys is a standard module, I think this is definitely a reasonable request that's on us by default.

That said, I completely agree with him that with the library review and stabilization effort we're undertaking (see slide 20 of these release notes https://chapel-lang.org/releaseNotes/1.23/05-ongoing.pdf), it is likely that these routines will be replaced with ones with a more Chapeltastic interface, so whether or not it's worth the immediate effort to fix this routine (given that there are probably other routines within the Sys module that would need similar documentation improvements) given that it's likely to change is a reasonable question.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/chapel-lang/chapel/issues/16705#issuecomment-728209110, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALGCG4632SRP727ZLHZEDTTSQFOHXANCNFSM4TV4BRBQ .

bradcray commented 4 years ago

If you want me to mark this issue as 'resolved' then I'm happy to do so.

Hi @tmacd8 — Thanks, I don't think there's any need to do so. I suspect we'll probably address the root issue by creating Chapel-y versions of these routines with their own documentation rather than putting effort into documenting the existing routines better, but it doesn't hurt to keep this open until then since it may help others understand what's going on here.

tmacd8 commented 4 years ago

Sounds good.

-- TMacD

On Fri, Nov 20, 2020 at 2:38 PM Brad Chamberlain notifications@github.com wrote:

If you want me to mark this issue as 'resolved' then I'm happy to do so.

Hi @tmacd8 https://github.com/tmacd8 — Thanks, I don't think there's any need to do so. I suspect we'll probably address the root issue by creating Chapel-y versions of these routines with their own documentation rather than putting effort into documenting the existing routines better, but it doesn't hurt to keep this open until then since it may help others understand what's going on here.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/chapel-lang/chapel/issues/16705#issuecomment-731395621, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALGCG42ETTKJ2LQNXFSHA2DSQ3HS3ANCNFSM4TV4BRBQ .

Koushikk-24 commented 4 years ago

Hi, I'm Rupesh and I'm interested in this issue can you please suggest me ways to get started

bradcray commented 4 years ago

Hi Rupesh. This issue is a case of needing to improve documentation for the sys_getenv routine. To get started with it, you should:

tushargoyl123 commented 3 years ago

Hi, I'm Tushar and I wanna cope with this issue can you guide me through the process...

bradcray commented 3 years ago

Hi @tushargoyl123 — We're on winter break through January 4th, so won't be able to be very responsive until then. Please see the guidance I gave @Rupeshkoushik just above and insure that they are not currently running with the issue so that neither of your time is wasted.

Yudhishthira1406 commented 3 years ago

Hi @bradcray I just wanted to know if the issue is still active and if I can work on it.

  • add documentation to chpl_getsys()

Also I didn't understand what you meant by the term chpl_getsys()

mppf commented 3 years ago

This issue is talking about the missing documentation here: https://chapel-lang.org/docs/master/modules/standard/Sys.html#Sys.sys_getenv

Yudhishthira1406 commented 3 years ago

@mppf @bradcray please review my PR #16996

bradcray commented 3 years ago

This was fixed in #16996. Thanks @tmacd8 for pointing out the lack of clarity, and @Yudhishthira1406 for adding the documentation.

tmacd8 commented 3 years ago

Great - glad that documentation is improved.

TMacD

On Thu, Jan 28, 2021, 19:34 Brad Chamberlain notifications@github.com wrote:

This was fixed in #16996 https://github.com/chapel-lang/chapel/pull/16996. Thanks @tmacd8 https://github.com/tmacd8 for pointing out the lack of clarity, and @Yudhishthira1406 https://github.com/Yudhishthira1406 for adding the documentation.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/chapel-lang/chapel/issues/16705#issuecomment-769512998, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALGCG424DE6ZPL6KCNZGKPDS4IGDBANCNFSM4TV4BRBQ .