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

Android setup #20

Closed
wants to merge 12 commits into from
Closed

Android setup #20

wants to merge 12 commits into from

Conversation

anderklander
Copy link
Contributor

@anderklander anderklander commented Jan 9, 2018

Setup to be able to test on android and moving some stuff to platform.js to be able to run it without importing electron.

At the moment the android build will fail on the SupportPage for still using electon, so don't expect anything fancy.

It would be very good if you could help me with testing logging and clipboard, since i dont have macOS and was not able to start the backend at the moment.

Based on the quit-button-position branch to get style changes.

Installation instruction draft:

Android

Follow the installation instructions for react-native. At the moment hot-loading android and desktop will not work at the same time (due to modules not found when using the app structure), content is copied from app/ to mobile/js/ when the run-android script is run. The andorid project can be either bundled in the apk or run with a packager server, settings for this is found in the build.gradle file in mobile/app/

The setup-android script installs the node_modules for the mobile project. (or just yarn install in the mobile/ dir)

yarn setup-android

To run and start logging. Will run on connected physical device or started emulator.

yarn run-android

This change is Reviewable

@eriklarko
Copy link
Contributor

Reviewed 129 of 130 files at r3.
Review status: 129 of 133 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@faern
Copy link
Member

faern commented Jan 10, 2018

Review status: 129 of 133 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


a discussion (no related file):
I know I'm not assigned to this PR, but I have some questions. I see that a lot of files that exist in the repo root are kind of duplicated under mobile/ (I have not compared content on most of them):
.gitignore, yarn.lock, .flowconfig etc.

Is this intentional? What makes this needed? Is the mobile/ directory a different "project" from some point of view?


Comments from Reviewable

@anderklander
Copy link
Contributor Author

Review status: 129 of 133 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


a discussion (no related file):

Previously, faern (Linus Färnstrand) wrote…

I know I'm not assigned to this PR, but I have some questions. I see that a lot of files that exist in the repo root are kind of duplicated under mobile/ (I have not compared content on most of them):
.gitignore, yarn.lock, .flowconfig etc.

Is this intentional? What makes this needed? Is the mobile/ directory a different "project" from some point of view?

That is for the react-native project, to be able to have different modules etc for electron vs mobile.


Comments from Reviewable

@faern
Copy link
Member

faern commented Jan 10, 2018

Review status: 129 of 133 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


a discussion (no related file):

Previously, anderklander wrote…

That is for the react-native project, to be able to have different modules etc for electron vs mobile.

Ok. So we need different node modules for desktop and mobile? Quite obvious when thinking about it. But does it have to be different node_modules/ for that to work?

I mean, in comparison, in Rust we do something like

[target.'cfg(windows)'.dependencies]
ctrlc = "3.0"

to depend on something on only a subset of the platforms. It will still exist in the global Cargo.lock, but it will only be included in the target platform on compilation.

Just thinking, since there will probably be a lot of overlap of modules as well? So having two node_modules/ requires more disk space and (might?) be harder to maintain?


Comments from Reviewable

@faern
Copy link
Member

faern commented Jan 10, 2018

Review status: 129 of 133 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


a discussion (no related file):

Previously, faern (Linus Färnstrand) wrote…

Ok. So we need different node modules for desktop and mobile? Quite obvious when thinking about it. But does it have to be different node_modules/ for that to work?

I mean, in comparison, in Rust we do something like

[target.'cfg(windows)'.dependencies]
ctrlc = "3.0"

to depend on something on only a subset of the platforms. It will still exist in the global Cargo.lock, but it will only be included in the target platform on compilation.

Just thinking, since there will probably be a lot of overlap of modules as well? So having two node_modules/ requires more disk space and (might?) be harder to maintain?

The .flowconfig is identical. Won't flow look upwards in the filesystem tree automatically if one does not exist in the local folder? Just thinking it's good if we can minimize duplication as it creates more maintenance. But I know very little about these specific tools so maybe not so helpful after all?


Comments from Reviewable

@pronebird
Copy link
Contributor

Please rebase on top of master since we merged some of PRs recently. That should slim down the diff we see here.


Review status: 129 of 133 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: 129 of 133 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


a discussion (no related file):

Previously, faern (Linus Färnstrand) wrote…

The .flowconfig is identical. Won't flow look upwards in the filesystem tree automatically if one does not exist in the local folder? Just thinking it's good if we can minimize duplication as it creates more maintenance. But I know very little about these specific tools so maybe not so helpful after all?

I agree with @faern. I don't understand why mobile/ needs an extra package. I thought the whole point of using ReactXP was to avoid that..


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: 129 of 133 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


package.json, line 78 at r4 (raw file):

    "test": "electron-mocha --renderer -R spec --require babel-core/register --require-main test/setup/main.js test/*.spec.js test/**/*.spec.js",
    "lint": "eslint --no-ignore scripts app test *.js",
    "lint-win": "eslint --rule 'linebreak-style': 0 --no-ignore scripts app test *.js",

It seems redundant to have two linters doing the same thing except using different EOL styles.


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: 129 of 133 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


package.json, line 73 at r4 (raw file):

  "scripts": {
    "setup-android": "cd mobile && yarn install",
    "run-android": "cp -r ./app/* ./mobile/js/ && cd mobile && yarn android",

Can we run from ./build?


Comments from Reviewable

@anderklander
Copy link
Contributor Author

Review status: 129 of 133 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


a discussion (no related file):

Previously, pronebird (Andrei Mihailov) wrote…

I agree with @faern. I don't understand why mobile/ needs an extra package. I thought the whole point of using ReactXP was to avoid that..

There are several "issues" with this setup. Its a trade-off between being able to run it for testing purposes as soon as possible and keeping the old structure etc. I suggest that we keep the discussion to concrete improvement that you know will work, or real issues, otherwise i think we could go on forever. I dont have any arguments for this setup other than that it works and i dont have any better idea that i have managed to get working in a relatively convenient matter, but if you know anything better, go ahead!


package.json, line 78 at r4 (raw file):

Previously, pronebird (Andrei Mihailov) wrote…

It seems redundant to have two linters doing the same thing except using different EOL styles.

Its a time saver for me.


Comments from Reviewable

@faern
Copy link
Member

faern commented Jan 11, 2018

Something went awry there. Looks like there was a merge instead of a rebase. Now this PR contains some commits that are already merged into master.


Review status: 123 of 143 files reviewed at latest revision, 3 unresolved discussions.


a discussion (no related file):

Previously, anderklander wrote…

There are several "issues" with this setup. Its a trade-off between being able to run it for testing purposes as soon as possible and keeping the old structure etc. I suggest that we keep the discussion to concrete improvement that you know will work, or real issues, otherwise i think we could go on forever. I dont have any arguments for this setup other than that it works and i dont have any better idea that i have managed to get working in a relatively convenient matter, but if you know anything better, go ahead!

Yeah. I suppose it's a big difference depending on if you view all of this as the mobile part being integrated into the project, or just attach to the side to easier test it? For the latter I agree all of this is fine, as long as we plan on merging everything tighter when we are ready to do so.


Comments from Reviewable

Update of scripts etc to run on android.
And change open to openLink/openItem in platform.js
Not able to start backend, so a little help with testing this would be good! Also good to test on specific platform i guess.
background -> backgroundColor and fontWeight value as string instead of number. Crasches on android …
@pronebird
Copy link
Contributor

Rebase should fix that I think.


Review status: 123 of 143 files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: 123 of 132 files reviewed at latest revision, 4 unresolved discussions.


a discussion (no related file):
I see that all assets are duplicated as PNG. This is not optimal and will go out of sync very quickly.

What I would do is make a script that converts SVG to PNG during the build.

I think all of our SVGs have the right size, this should be trivial to produce the images then for iOS and Android, i.e @2x, @3x and kind of HD images needed for Android.


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: 123 of 132 files reviewed at latest revision, 9 unresolved discussions.


mobile/App.android.js, line 34 at r5 (raw file):

});

MobileAppBridge.startBackend().then(response => {})

Same here. Needs to be abstracted in order to avoid separating logic.


Comments from Reviewable

@pronebird
Copy link
Contributor

Reviewed 124 of 131 files at r3, 9 of 9 files at r5.
Review status: all files reviewed at latest revision, 9 unresolved discussions.


Comments from Reviewable

@faern
Copy link
Member

faern commented Jan 11, 2018

Review status: all files reviewed at latest revision, 9 unresolved discussions.


a discussion (no related file):

Previously, pronebird (Andrei Mihailov) wrote…

I went through the simple guide of adding react-native into an existing android app:

https://facebook.github.io/react-native/docs/integration-with-existing-apps.html

I don't know all specifics but I assume you've created an Android project and integrated react-native in it. If that so, I think we could add a few commands in package.json and there is absolutely no need to copy anything and have separate package.json since we already have react-native included in our package.json.

If we can make it simpler, that is of course desired. At the same time we have to realize that it's a huge task to get it to work on mobile etc. So I fully understand if we need some period of time where we kind of have them tag along next to each other before we help each other get it into one unified package/project.
It depends on how easy it is to fix the things you are suggesting now vs getting it done later.


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: all files reviewed at latest revision, 9 unresolved discussions.


a discussion (no related file):

Previously, faern (Linus Färnstrand) wrote…

If we can make it simpler, that is of course desired. At the same time we have to realize that it's a huge task to get it to work on mobile etc. So I fully understand if we need some period of time where we kind of have them tag along next to each other before we help each other get it into one unified package/project.
It depends on how easy it is to fix the things you are suggesting now vs getting it done later.

I agree. I can disregard a copy of App.js that does less stuff than on desktop and image assets for now.


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: all files reviewed at latest revision, 10 unresolved discussions.


mobile/rust/mullvad/copy_android.sh, line 3 at r5 (raw file):

#! /bin/bash
mkdir -p ../../android/app/src/main/jniLibs
mkdir -p ../../android/app/src/main/jniLibs/x86

You don't need -p here and all subsequent calls to mkdir since you've already created a parent folder with -p in the first place and all subfolders you create are direct children of jniLibs.


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: all files reviewed at latest revision, 10 unresolved discussions.


a discussion (no related file):

Previously, pronebird (Andrei Mihailov) wrote…

I agree. I can disregard a copy of App.js that does less stuff than on desktop and image assets for now.

But we need to sort out the configuration for mobile. I don't even understand why mobile/package.json uses different versions of the same packages we use on desktop. That's a tech debt. I can't accept that.


Comments from Reviewable

@anderklander
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 10 unresolved discussions.


a discussion (no related file):

Previously, pronebird (Andrei Mihailov) wrote…

But we need to sort out the configuration for mobile. I don't even understand why mobile/package.json uses different versions of the same packages we use on desktop. That's a tech debt. I can't accept that.

It might have changed now since we are converting things to reactxp and perhaps soon have removed problematic packages, but it was hard to solve in a good way when i tested in the beginning of the project. Its catch 22, in order to have everything working with the same packages everything (at least there were a lot of things back then) will have to be rewritten, in order to rewrite everything i suppose it is better to have some kind of environment to be able to test the individual changes? The plan with this was to enable someone else to build for android and hopefully will be able to avoid adding new issues due to the quirks of this cross platform thing, otherwise all my time will be spent on trying to keep up with your changes at the same time as trying to rewrite and get a functional android app. This is very much not a good final project setup, but i think that it could help us get to that point faster.


Comments from Reviewable

@anderklander
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 10 unresolved discussions.


package.json, line 78 at r4 (raw file):

Previously, pronebird (Andrei Mihailov) wrote…

Could you elaborate why do you need this? We have all source code passing linter on Linux and MacOS. Why it doesn't work for you?

Since i am using win-style endings in my editor, and git handles the conversion. If i run lint with the line ending set i will get one error per line, not very user friendly.


Comments from Reviewable

@anderklander
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 10 unresolved discussions.


mobile/App.android.js, line 27 at r5 (raw file):

Previously, pronebird (Andrei Mihailov) wrote…

Can we abstract stuff like this and combine both Electron events and DeviceEventEmitter events into one class to avoid code duplication?

For example you can create an interface in Flow:

interface PlatformEventEmitter {
  addEventListener(event: string, fn: () => Void);
  removeEventListener(fn: () => Void);
}

Then you could implement the bridge for Electron and DeviceEventEmitter in separate files conforming to single interface.

This gives guarantees and react-native picks up the implementation by platform.

We probably can, someone who knows electron and the backend would probably be a good candidate since that is the only really important thing to work right now ... but seems like a waste of time at the moment for me to try to do it.


mobile/App.android.js, line 34 at r5 (raw file):

Previously, pronebird (Andrei Mihailov) wrote…

Same here. Needs to be abstracted in order to avoid separating logic.

I would suggest that we keep it to two separate files until someone has even started working on a proper backend on android and we can make a more educated guess on the possible need for two files or not.


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: all files reviewed at latest revision, 10 unresolved discussions.


package.json, line 78 at r4 (raw file):

Previously, anderklander wrote…

Since i am using win-style endings in my editor, and git handles the conversion. If i run lint with the line ending set i will get one error per line, not very user friendly.

This is something that can be solved with the proper editor configuration. For example Sublime Text supports that.


Comments from Reviewable

@anderklander
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 10 unresolved discussions.


a discussion (no related file):

Previously, faern (Linus Färnstrand) wrote…

Are those the built versions of this sandbox android-rust crate? Yes those probably should not be commited.

I can remove them, just figured that it would be much faster to skip installing and adding instructions for building rust/golang things right now. Also removes the need to install the android NDK just to be able to test at the moment.


Comments from Reviewable

@anderklander
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 8 unresolved discussions.


package.json, line 78 at r4 (raw file):

Previously, pronebird (Andrei Mihailov) wrote…

This is something that can be solved with the proper editor configuration. For example Sublime Text supports that.

Yes. But i have other projects where i do not want to have that setting. And having different settings for different projects takes time. But i can continue to waste time and energy if its important not to have that line.


Comments from Reviewable

@faern
Copy link
Member

faern commented Jan 11, 2018

Review status: all files reviewed at latest revision, 8 unresolved discussions.


a discussion (no related file):

Previously, anderklander wrote…

I can remove them, just figured that it would be much faster to skip installing and adding instructions for building rust/golang things right now. Also removes the need to install the android NDK just to be able to test at the moment.

If we should be able to test modifying it we would need all those things anyway, so would be good to get the documentation going anyway. And sadly Rust binaries are fairly huge, so it's not optimal to have them in git (no binaries are)
Testing the Rust part without actually playing with the code feels like a limited use-case?

What part is golang? Do we really need to bring in another language?


Comments from Reviewable

@faern
Copy link
Member

faern commented Jan 11, 2018

Review status: all files reviewed at latest revision, 8 unresolved discussions.


a discussion (no related file):

Previously, anderklander wrote…

It might have changed now since we are converting things to reactxp and perhaps soon have removed problematic packages, but it was hard to solve in a good way when i tested in the beginning of the project. Its catch 22, in order to have everything working with the same packages everything (at least there were a lot of things back then) will have to be rewritten, in order to rewrite everything i suppose it is better to have some kind of environment to be able to test the individual changes? The plan with this was to enable someone else to build for android and hopefully will be able to avoid adding new issues due to the quirks of this cross platform thing, otherwise all my time will be spent on trying to keep up with your changes at the same time as trying to rewrite and get a functional android app. This is very much not a good final project setup, but i think that it could help us get to that point faster.

If this is mostly a way to share the sandbox/testing of the app and for multiple people to be able to help with it. Then I don't see a need to actually merge this. It would be very easy to just have it as a feature branch that people workning on mobile can collaborate on. Then when the mobile part of the project get so far that we know more what we are doing, then we can try to integrate it with the rest of the project. If it's more or less a separate folder anyway it should be easy to rebase and it also does not benefit much from being merged.


Comments from Reviewable

@faern
Copy link
Member

faern commented Jan 11, 2018

Review status: all files reviewed at latest revision, 8 unresolved discussions.


a discussion (no related file):

Previously, faern (Linus Färnstrand) wrote…

If this is mostly a way to share the sandbox/testing of the app and for multiple people to be able to help with it. Then I don't see a need to actually merge this. It would be very easy to just have it as a feature branch that people workning on mobile can collaborate on. Then when the mobile part of the project get so far that we know more what we are doing, then we can try to integrate it with the rest of the project. If it's more or less a separate folder anyway it should be easy to rebase and it also does not benefit much from being merged.

With such sandbox branches it's no problem having committed binaries etc. Much less requirement on keeping it tidy. So maybe that would be the easiest?


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: all files reviewed at latest revision, 8 unresolved discussions.


package.json, line 78 at r4 (raw file):

Previously, anderklander wrote…

Yes. But i have other projects where i do not want to have that setting. And having different settings for different projects takes time. But i can continue to waste time and energy if its important not to have that line.

It's important to not pollute the project with personal configurations.

There is absolutely no need to waste time and nobody encourages you to do that.

The solution is to add the editor configuration for that specific project. For example VSCode supports workspace configuration (it creates hidden .vscode folder).

Depending on what editor you use to work with Javascript you may be able to achieve this either via GUI or dot file. Many editors support that. If dot file appears in git staging, make sure you add it to .gitignore.


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: all files reviewed at latest revision, 8 unresolved discussions.


package.json, line 78 at r4 (raw file):

Previously, pronebird (Andrei Mihailov) wrote…

It's important to not pollute the project with personal configurations.

There is absolutely no need to waste time and nobody encourages you to do that.

The solution is to add the editor configuration for that specific project. For example VSCode supports workspace configuration (it creates hidden .vscode folder).

Depending on what editor you use to work with Javascript you may be able to achieve this either via GUI or dot file. Many editors support that. If dot file appears in git staging, make sure you add it to .gitignore.

Alternatively you could always pass the rule in the call to lint, i.e:

yarn lint --rule 'linebreak-style': 0

Many shells support history so you don't have to type it each type. Or maybe there is a shortcut you can use.


Comments from Reviewable

@anderklander
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 8 unresolved discussions.


a discussion (no related file):

Previously, faern (Linus Färnstrand) wrote…

Yeah. I suppose it's a big difference depending on if you view all of this as the mobile part being integrated into the project, or just attach to the side to easier test it? For the latter I agree all of this is fine, as long as we plan on merging everything tighter when we are ready to do so.

Took another fight with the module monster last evening, and i think it might be possible to merge the projects closer quite soon (we have removed most packages that was causing the issues, and the rest might be possible to resolve quite easy). Will look further today. I guess it seems like i am trying to add a lot of crap, and i am probably doing that because it feels impossible to keep up with verifying changes and getting forward when "fixed" files becomes broken almost immediately, i know that this is a problem that will go away when the quirks of using reactxp etc has been found and we are getting used to it.


a discussion (no related file):

Previously, faern (Linus Färnstrand) wrote…

If we should be able to test modifying it we would need all those things anyway, so would be good to get the documentation going anyway. And sadly Rust binaries are fairly huge, so it's not optimal to have them in git (no binaries are)
Testing the Rust part without actually playing with the code feels like a limited use-case?

What part is golang? Do we really need to bring in another language?

Wireguard, at least the implementation that seems to be the best candidate at the moment. I would be the first to jump in joy if we could manage without another language.


package.json, line 78 at r4 (raw file):

Previously, pronebird (Andrei Mihailov) wrote…

Alternatively you could always pass the rule in the call to lint, i.e:

yarn lint --rule 'linebreak-style': 0

Many shells support history so you don't have to type it each type. Or maybe there is a shortcut you can use.

If we use a .gitattributes with eol settings in the repo it should work ok, but i assumed that to be more intrusive and as much of a personal configuration as this line. Its ok if you dont want it and good if you have arguments against it, but i would feel better if you could take my word on that i would save time using it instead of implying that i am just misinformed and telling me stuff that i obviously already know (since i wrote the line that you are suggesting me to use). I will remove it.


Comments from Reviewable

@anderklander
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 8 unresolved discussions.


a discussion (no related file):

Previously, pronebird (Andrei Mihailov) wrote…

I see that all assets are duplicated as PNG. This is not optimal and will go out of sync very quickly.

What I would do is make a script that converts SVG to PNG during the build.

I think all of our SVGs have the right size, this should be trivial to produce the images then for iOS and Android, i.e @2x, @3x and kind of HD images needed for Android.

Yes, the plan is to use a script of some kind.


Comments from Reviewable

@anderklander
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 8 unresolved discussions.


package.json, line 78 at r4 (raw file):

Previously, anderklander wrote…

If we use a .gitattributes with eol settings in the repo it should work ok, but i assumed that to be more intrusive and as much of a personal configuration as this line. Its ok if you dont want it and good if you have arguments against it, but i would feel better if you could take my word on that i would save time using it instead of implying that i am just misinformed and telling me stuff that i obviously already know (since i wrote the line that you are suggesting me to use). I will remove it.

Sorry, not the exact line, was reading too early.


Comments from Reviewable

@faern
Copy link
Member

faern commented Jan 12, 2018

Review status: all files reviewed at latest revision, 8 unresolved discussions.


a discussion (no related file):

Previously, anderklander wrote…

Took another fight with the module monster last evening, and i think it might be possible to merge the projects closer quite soon (we have removed most packages that was causing the issues, and the rest might be possible to resolve quite easy). Will look further today. I guess it seems like i am trying to add a lot of crap, and i am probably doing that because it feels impossible to keep up with verifying changes and getting forward when "fixed" files becomes broken almost immediately, i know that this is a problem that will go away when the quirks of using reactxp etc has been found and we are getting used to it.

The "impossible to keep up" part, are you referring to keeping up with our moving master branch, or keeping up with something else? See my comment on whether or not merging this PR at all, if it's mostly for testing. But if the trouble is keeping up with master, then there might be real advantage in having it merged of course.


a discussion (no related file):

Previously, anderklander wrote…

Wireguard, at least the implementation that seems to be the best candidate at the moment. I would be the first to jump in joy if we could manage without another language.

Aah. I see. But we won't have wireguard in this repo? We'll likely pull it in in some way similar to client-binaries where we have that stuff external and just bundle the binaries we need to call in the build step, not actually having them here.
So if you need a wireguard binary somewhere we should probably commit it to client-binaries.


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: all files reviewed at latest revision, 8 unresolved discussions.


package.json, line 78 at r4 (raw file):

Previously, anderklander wrote…

Sorry, not the exact line, was reading too early.

You don't have to commit that line to repository if other solutions that I suggested to you don't work for you.

For example I use git stash to apply certain patches to the source code to help me when developing but then I never commit the actual changes to the repo. The stash is saved in git and can be applied at any time, then you can reset it before committing.

What editor do you use for JavaScript? Maybe I can help you to figure out how to add the configuration so you could develop using the same EOL.


Comments from Reviewable

@anderklander
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 8 unresolved discussions.


a discussion (no related file):

Previously, faern (Linus Färnstrand) wrote…

The "impossible to keep up" part, are you referring to keeping up with our moving master branch, or keeping up with something else? See my comment on whether or not merging this PR at all, if it's mostly for testing. But if the trouble is keeping up with master, then there might be real advantage in having it merged of course.

Keeping up with master, but i will reorganize and split this, and hopefully soon be able to add a better setup.


package.json, line 78 at r4 (raw file):

Previously, pronebird (Andrei Mihailov) wrote…

You don't have to commit that line to repository if other solutions that I suggested to you don't work for you.

For example I use git stash to apply certain patches to the source code to help me when developing but then I never commit the actual changes to the repo. The stash is saved in git and can be applied at any time, then you can reset it before committing.

What editor do you use for JavaScript? Maybe I can help you to figure out how to add the configuration so you could develop using the same EOL.

I know that i can stash etc. and i have no issues with line endings or changing settings for either editor or git or anything else. But if you want to force editor usage for other developers i suggest that a .gitattributes file for that purpose is used (if its working that way, have not tested since i prefer to use native line endings for the OS that is running, since git conversions always has worked for me).


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: all files reviewed at latest revision, 7 unresolved discussions.


package.json, line 78 at r4 (raw file):

Previously, anderklander wrote…

I know that i can stash etc. and i have no issues with line endings or changing settings for either editor or git or anything else. But if you want to force editor usage for other developers i suggest that a .gitattributes file for that purpose is used (if its working that way, have not tested since i prefer to use native line endings for the OS that is running, since git conversions always has worked for me).

I mentioned two different editors and never said you are obliged to use one or another.

I suggested that you could configure your editor on per-project basis which is usually done via dot files.

Can this be done outside of the repository?


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: all files reviewed at latest revision, 7 unresolved discussions.


a discussion (no related file):

Previously, faern (Linus Färnstrand) wrote…

Aah. I see. But we won't have wireguard in this repo? We'll likely pull it in in some way similar to client-binaries where we have that stuff external and just bundle the binaries we need to call in the build step, not actually having them here.
So if you need a wireguard binary somewhere we should probably commit it to client-binaries.

Are we going wireguard first? Felt like if we have everything working with openvpn maybe we should implement it first?


Comments from Reviewable

@faern
Copy link
Member

faern commented Jan 12, 2018

Review status: all files reviewed at latest revision, 7 unresolved discussions.


a discussion (no related file):

Previously, pronebird (Andrei Mihailov) wrote…

Are we going wireguard first? Felt like if we have everything working with openvpn maybe we should implement it first?

@pronebird We are not going to support OpenVPN at all on mobile. It's going to be wireguard only. That has been the plan from the start.


Comments from Reviewable

@pronebird
Copy link
Contributor

Review status: all files reviewed at latest revision, 7 unresolved discussions.


a discussion (no related file):

Previously, faern (Linus Färnstrand) wrote…

@pronebird We are not going to support OpenVPN at all on mobile. It's going to be wireguard only. That has been the plan from the start.

@faern not sure which "start" you reference to, because a year ago Wireguard was merely a concept. In any case I am happy that things change for mobile.


Comments from Reviewable

@faern
Copy link
Member

faern commented Jan 12, 2018

Review status: all files reviewed at latest revision, 7 unresolved discussions.


a discussion (no related file):

Previously, pronebird (Andrei Mihailov) wrote…

@faern not sure which "start" you reference to, because a year ago Wireguard was merely a concept. In any case I am happy that things change for mobile.

Ever since we brought mobile into the picture. When we started we said we will do mobile in the future, but we had no plans at all more or less. Some months ago when we started planning the mobile part we said it will be wireguard only.
Simply because wireguard in userspace will likely be released before we get to that point anyway and it's sooo much better for battery driven devices.


Comments from Reviewable

@anderklander anderklander deleted the android-setup branch April 12, 2018 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants