Add platform: module to expose platform data #927

krader1961 · 2020-02-28T00:56:54Z

Resolves #916

xiaq · 2020-03-08T23:03:02Z

pkg/eval/sys/sys.go

+
+func init() {
+	Ns.Add("arch", vars.NewReadOnly(runtime.GOARCH))
+	Ns.Add("platform", vars.NewReadOnly(runtime.GOOS))


I imagine you called it after Python's sys.platform, but I'd rather this be called os to align with Go.

I did but on second thought agree with using os for consistency given how tightly Elvish is bound to Go.

I also assume you'll want the namespace changed from sys to os.

And the straightforward rename exposes a problem:

pkg/program/shell/runtime.go:18:2: os redeclared as imported package name previous declaration at pkg/program/shell/runtime.go:7:2

Which means that any import of the elvish os module must rename it. Which means we now need to establish a convention for this situation. 😦 I have tentatively settled on importing elvish_os "github.com/elves/elvish/pkg/eval/os".

My preference is calling this module sys; it is a bit awkward to have a variable called $os:os, no?

Note that my first incarnation did call this the sys: namespace 😄 Not sure what you mean by your $os:os example. There is no such var at this time. There is $os:platform and $os:arch. I deliberately used platform because os has too much baggage. For example, why is the value darwin rather than macos on my Macs? Or linux rather than ubuntu? Etcetera. I think platform better conveys the meaning of this var.

pkg/eval/sys/sys_test.go

pkg/eval/sys/sys.go

website/ref/sys.md

pkg/eval/sys/sys.go

pkg/eval/sys/sys_test.go

xiaq · 2020-03-08T23:22:08Z

pkg/eval/sys/sys.go

@@ -0,0 +1,25 @@
+// Package sys exposes variables and functions that deal with the specific


I am considering putting these variables related to interpreter internals in this module too:

elvish/pkg/eval/eval.go

Lines 148 to 162 in 7868b68

beforeChdirElvish, afterChdirElvish := vector.Empty, vector.Empty

ev.beforeChdir = append(ev.beforeChdir,

adaptChdirHook("before-chdir", ev, &beforeChdirElvish))

ev.afterChdir = append(ev.afterChdir,

adaptChdirHook("after-chdir", ev, &afterChdirElvish))

builtin["before-chdir"] = vars.FromPtr(&beforeChdirElvish)

builtin["after-chdir"] = vars.FromPtr(&afterChdirElvish)

builtin["value-out-indicator"] = vars.FromPtrWithMutex(

&ev.state.valuePrefix, &ev.state.mutex)

builtin["notify-bg-job-success"] = vars.FromPtrWithMutex(

&ev.state.notifyBgJobSuccess, &ev.state.mutex)

builtin["num-bg-jobs"] = vars.FromGet(func() interface{} {

return strconv.Itoa(ev.state.getNumBgJobs())

})

Which won't fit this description something along the description Python's sys module would be more accurate:

This module provides access to some variables used or maintained by the interpreter and to functions that interact strongly with the interpreter.

My preference is for anything that is explicitly tied to the OS or architecture, and which cannot be papered over by an OS agnostic API, be in the os namespace rather than in the builtin namespace.

krader1961 · 2020-03-09T23:41:20Z

@xiaq Please take another look.

xiaq · 2020-03-18T23:49:36Z

I posted a comment, but actually I think we need to think harder on how to name this module and what its scope should be.

I was considering modelling the module after Python's sys (and commented that I preferred it), or Go's runtime, but after thinking about it more, I think both make the mistake of mixing at least two kinds of things:

Things important to the runtime environment itself: Python has sys.path; Go has runtime.GOROOT.
Things related to the low-level environment: Python has sys.platform, Go has runtime.GOOS.

I think it's wise to separate both things, and the variables in this PR definitely belong to the second bullet. Your proposed name of os comes close to describing it, but I'm not enthusiastic about the name. Not because of the clash with Go's os, but because $os:os is too awkward.

Maybe platform is a good choice? Variable names $platform:os and $platform:arch make a lot of sense. It is slightly too long, though.

(I am inspired by Python's platform module, although the Python module makes the mistake of including information about the interpreter.)

It's probably also worth researching how other languages call such modules.

krader1961 · 2020-03-19T01:06:29Z

I think we need to think harder on how to name this module and what its scope should be.

Okay, but since it seems like we're still a ways away from a elvish 1.0 release it seems like doing something reasonable, if not ideal, is better than not doing anything and forcing people to use the uname command. As people use it and provide feedback we can refactor as appropriate.

Maybe platform is a good choice? Variable names $platform:os and $platform:arch make a lot of sense. It is slightly too long, though.

I'm not bothered by the length of those two example var names. However, I was thinking that builtins like chmod and umask which cannot be implemented in an OS agnostic fashion would be placed in the os: namespace. That, in fact, is what Python does. Perhaps we should start with a platform: namespace that exposes basic facts (e.g., the CPU architecture) then add an os: namespace for OS specific capabilities like umask.

xiaq · 2020-03-19T09:22:48Z

@krader1961 I agree with what you said, so let's rename the module in this PR to platform and we are good to go.

Resolves elves#916

krader1961 · 2020-03-19T23:34:33Z

@xiaq, Please take another look.

xiaq

A few final nitpicks, almost ready to go.

pkg/eval/platform/platform.go

pkg/eval/platform/platform_test.go

website/ref/index.toml

website/ref/platform.md

krader1961 mentioned this pull request Feb 28, 2020

Initial crude attempt at adding a built-in variable which returns the runtime OS #913

Closed

xiaq reviewed Mar 8, 2020

View reviewed changes

krader1961 force-pushed the add-sys-module branch from 15485ae to c8fd862 Compare March 9, 2020 23:40

krader1961 force-pushed the add-sys-module branch from c8fd862 to b004f71 Compare March 10, 2020 02:20

krader1961 mentioned this pull request Mar 17, 2020

Add umask command #681

Closed

krader1961 changed the title ~~Add sys: module to expose platform and arch vars~~ Add os: module to expose platform and arch vars Mar 17, 2020

Add platform: module to expose platform data

881ccf9

Resolves elves#916

krader1961 force-pushed the add-sys-module branch from b004f71 to 881ccf9 Compare March 19, 2020 23:32

krader1961 changed the title ~~Add os: module to expose platform and arch vars~~ Add platform: module to expose platform data Mar 19, 2020

xiaq reviewed Mar 19, 2020

View reviewed changes

Cleanup after renaming the package

d498bc3

xiaq merged commit a7ed45f into elves:master Mar 19, 2020

krader1961 deleted the add-sys-module branch March 20, 2020 00:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add platform: module to expose platform data #927

Add platform: module to expose platform data #927

krader1961 commented Feb 28, 2020

xiaq Mar 8, 2020

krader1961 Mar 9, 2020

krader1961 Mar 9, 2020

krader1961 Mar 9, 2020

xiaq Mar 18, 2020

krader1961 Mar 19, 2020

xiaq Mar 8, 2020

krader1961 Mar 9, 2020

krader1961 commented Mar 9, 2020

xiaq commented Mar 18, 2020

krader1961 commented Mar 19, 2020

xiaq commented Mar 19, 2020

krader1961 commented Mar 19, 2020

xiaq left a comment

		@@ -0,0 +1,25 @@
		// Package sys exposes variables and functions that deal with the specific

	beforeChdirElvish, afterChdirElvish := vector.Empty, vector.Empty
	ev.beforeChdir = append(ev.beforeChdir,
	adaptChdirHook("before-chdir", ev, &beforeChdirElvish))
	ev.afterChdir = append(ev.afterChdir,
	adaptChdirHook("after-chdir", ev, &afterChdirElvish))
	builtin["before-chdir"] = vars.FromPtr(&beforeChdirElvish)
	builtin["after-chdir"] = vars.FromPtr(&afterChdirElvish)

	builtin["value-out-indicator"] = vars.FromPtrWithMutex(
	&ev.state.valuePrefix, &ev.state.mutex)
	builtin["notify-bg-job-success"] = vars.FromPtrWithMutex(
	&ev.state.notifyBgJobSuccess, &ev.state.mutex)
	builtin["num-bg-jobs"] = vars.FromGet(func() interface{} {
	return strconv.Itoa(ev.state.getNumBgJobs())
	})

Add platform: module to expose platform data #927

Add platform: module to expose platform data #927

Conversation

krader1961 commented Feb 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krader1961 commented Mar 9, 2020

xiaq commented Mar 18, 2020

krader1961 commented Mar 19, 2020

xiaq commented Mar 19, 2020

krader1961 commented Mar 19, 2020

xiaq left a comment

Choose a reason for hiding this comment