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

Add config path for windows OS #75

Merged
merged 11 commits into from
Mar 3, 2022
Merged

Conversation

morrieinmaas
Copy link
Contributor

Signed-off-by: morrieinmaas [email protected]

closes #54

@morrieinmaas morrieinmaas added the enhancement New feature or request label Feb 28, 2022
@morrieinmaas
Copy link
Contributor Author

It's WIP @blu3beri . Need to test that first. Windows VM being a right b*** atm.

@morrieinmaas morrieinmaas self-assigned this Feb 28, 2022
@morrieinmaas morrieinmaas marked this pull request as draft February 28, 2022 15:47
@morrieinmaas morrieinmaas force-pushed the feat/windows-config-file branch from 31fdd7f to c7996c1 Compare February 28, 2022 16:00
@morrieinmaas morrieinmaas force-pushed the feat/windows-config-file branch from 4d35750 to fa3e523 Compare March 1, 2022 12:45
Signed-off-by: morrieinmaas <[email protected]>
@morrieinmaas morrieinmaas force-pushed the feat/windows-config-file branch from fa3e523 to a8a3b1a Compare March 1, 2022 12:47
Signed-off-by: morrieinmaas <[email protected]>
@berendsliedrecht
Copy link
Member

Approved when tested.

@morrieinmaas
Copy link
Contributor Author

@blu3beri Just a little update:
The good news:

  • works on WSL

The bad news:
This doesn't quite work yet for windows powershell/native. There are several problems:

  • When compiling the switch checking for cfg!(windows) works but the compiler isn't happy with HOME not being defined (even though that is just for unix). Maybe we need to add sth like compile target or sth as a switch?
  • %AppData% just creates a new folder of that name in the project root -> compiles but config not found then
  • Also found that apparnelty %USERDATA% is a place for config files / the equivalent of unix $HOME -> results in the same behaviour as the previous error.

I'll continue to figure this out (now that I have a WIndows machine up and running). Just have no clue how Windows works so need to do some digging.

On another note, I needed to install some dependencies like C++ compiler/build env for windows for this to compile in the first place. Maybe we can open a ticket about this. I'm just not sure how to articulate this properly (yet) as again I don't know how windows works and how to define dependencies for a rust project for windows compile or whether to just set that in the README and let people know and then that's enough.🤷‍♂️😅

@berendsliedrecht
Copy link
Member

berendsliedrecht commented Mar 2, 2022

Okay, yeah the %...% was a small chance that it would work. It is the same as how "~" works in unix, in the sense that it expands to another full path, like /users/foo

On the compiling step, it is not really a problem. People who want to contribute to a rust project must deal with them themselves, like with any language. And the binary comes, 99.999% sure, with all the bells and whistles to work.

I do think the error about home not being specified is odd. Could you send a screenshot of that?

This works with WSL and Powershell

Signed-off-by: morrieinmaas <[email protected]>
@morrieinmaas morrieinmaas changed the title [WIP] feat: WIP (untested) Add config path for windows OS Add config path for windows OS Mar 3, 2022
@morrieinmaas
Copy link
Contributor Author

@blu3beri let me know if you have any more feedback. This works now for windows (and doesn't break unix.

One thing I see is we can probably extract the logic for path creation to the config file inot a function as it's the same code in 2 places now. Can do that here or in another PR. (or not at all if you don't see fit)

cli/src/modules/configuration.rs Outdated Show resolved Hide resolved
Only create path for unix and windows. Throw and error if neither OS type can be detected.

Signed-off-by: morrieinmaas <[email protected]>
@morrieinmaas morrieinmaas force-pushed the feat/windows-config-file branch from ba140b3 to a02a871 Compare March 3, 2022 11:34
@morrieinmaas morrieinmaas marked this pull request as ready for review March 3, 2022 11:36
@morrieinmaas
Copy link
Contributor Author

Alright @blu3beri

Signed-off-by: morrieinmaas <[email protected]>
@morrieinmaas morrieinmaas force-pushed the feat/windows-config-file branch from a02a871 to c882d7c Compare March 3, 2022 11:40
@berendsliedrecht berendsliedrecht merged commit 3e9c8ee into main Mar 3, 2022
@berendsliedrecht berendsliedrecht deleted the feat/windows-config-file branch March 3, 2022 14:03
jl-animo pushed a commit that referenced this pull request Mar 4, 2022
* main:
  fix: config path for windows OS (#75)
  feat: better check on attribute length (#80)
  fix: improved error message for config and connection without flag (#74)
  fix: print the connection id (#77)
  fix: schema requires attributes (#78)
  fix: american english (#79)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows testing
2 participants