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

Importing os module for use in macros fails when compiling for platform that doesn't support os #19414

Closed
PMunch opened this issue Jan 18, 2022 · 7 comments · Fixed by #22278 · May be fixed by #24639
Closed

Importing os module for use in macros fails when compiling for platform that doesn't support os #19414

PMunch opened this issue Jan 18, 2022 · 7 comments · Fixed by #22278 · May be fixed by #24639

Comments

@PMunch
Copy link
Contributor

PMunch commented Jan 18, 2022

Example

import os

macro someMacro(): untyped =
  result = newStmtList()
  for _ in walkDir("somedir"):
    discard

Current Output

When compiling for --os:any

Error: OS module not ported to your operating system!

Expected Output

No error, should be able to use walkDir and other things from os in a macro even though the target platform doesn't support the module. The module is explicitly mentioned to work with NimScript, so using it in this way should work.

Possible Solution

Maybe support static: import os to import it only into the static context where it could be used in macros. Or move the check to error out when procedures are actually used at runtime.

@beef331
Copy link
Collaborator

beef331 commented Jan 18, 2022

I think this works as intended but I've also walked into wanting to run the VM for my host OS and not target so perhaps a hostStatic and {.host.} pragma could be implemented to allow running it as if host OS instead of target. Compile time is supposed to after all emulate the target, but sometimes one might want to override it.

@PMunch
Copy link
Contributor Author

PMunch commented Jan 18, 2022

Why should compile time emulate the target though? Compile time is compile time, and should use whatever the compiling machine is defined as. You could easily run into a similar issue if you're cross compiling for example, it could end up trying to import something which won't work on compile-time simply because you're targeting another platform.

@beef331
Copy link
Collaborator

beef331 commented Jan 18, 2022

The possible issue I see is that if your VM is being ran as if a different OS specific symbols you attempt to fetch may not be available to the target, causing compilation errors.
Say you have the following:

when defined(linux):
  proc doThing() =
    someCompileTimeProcThatOnlyWorksOnLinux()
    echo "hello"
elif defined(windows):
  proc doThing() =
    someCompileTimeProcThatOnlyWorksOnWindows()
    echo "World"

static: 
  doThing()

Which doThing is correct to call if compiling on a linux machine to windows? I'd say it's the "hello" one assuming your rules, but the code does not know of it(we have windows not linux defined), so now we need to recompile all modules for linux as well, so we know all the symbols we have. This is an issue for both my suggestion and yours, so I really do not know what is best. Perhaps a import std/os {.hostOs.} which imports the module at compile time only and defines the hostOs inside. Running the VM as a different OS just screams longer compile times to me(especially with the fact marshal is still inside the NimVM).

Edit: At least in this case the solution is just to implement the vmOps in system instead of os or somewhere that the VM doesnt need to import.

@metagn
Copy link
Collaborator

metagn commented Jan 19, 2022

IMO VM-only overrides should be their own symbols and not depend on existing standard library functions. This is a pain point for JS but I don't think it'd be a huge problem for the VM given how much less commonly these functions are used.

@PMunch
Copy link
Contributor Author

PMunch commented Jan 19, 2022

@beef331, this isn't exclusively an issue for the os module, I seem to remember having this error with other modules as well (and it'd be trivial to write another module in a library which behaved the same way). So I don't believe a special case for OS is really the best way of doing it. I imagine this is even a problem for the JS backend as it would also try to pick the JS/Node way of doing things from within a macro.

@Araq
Copy link
Member

Araq commented Jan 20, 2022

@metagn That's what we used to do but people found it too confusing, so we got a new policy "just make everything work at compile-time". I don't like this policy at all but I also don't want to change it yet again and break people's code.

@PMunch
Copy link
Contributor Author

PMunch commented Feb 16, 2022

This is really starting to frustrate me.. I just had to go digging with strace to figure out which module in polymorph imported the os module. Apparently it was random which imports it to support seeding the random generator without explicitly passing a seed. It didn't matter that those functions from the random module wasn't used in polymorph because as soon as something even touches the os module it errors out. Luckily the random module has the -d:standalone flag, so I was at least able to fix it. But an issue which is unfixable is trying to use -d:ecsLogCode which should show what the polymorph macros generate because it uses the os module path joining logic in a macro to write nicer outputs (which should be supported, because we're not yet running on the targeted OS).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment