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

disable https enforcement in debug mode #2

Open
wants to merge 1 commit into
base: spm
Choose a base branch
from

Conversation

balland
Copy link

@balland balland commented Feb 12, 2019

Found in https://alexplescan.com/posts/2016/05/03/swift-a-nicer-way-to-tell-if-your-app-is-running-in-debug-mode/

As @ajandrade said, let's hope this hack is not going to disappear in
the next version of Swift. Anyway we will need to update our
dependencies and see if all our changes still apply :)

This allows us to connect to the dev environment without https.

Related to sensational/popscan-ios#1358
@brainlock
Copy link
Member

I'm not sure I'm the most appropriate reviewer for this, but from other languages I would say: what's wrong with a preprocessor check (macro?) instead of a runtime check? Calls to private APIs might be detected unless the compiler optimizes it away, with a preprocessor macro we'd be sure it gets removed

@balland
Copy link
Author

balland commented Feb 12, 2019

This is what I wanted to do first and then I realised that this dependencies are built by carthage so this means we would need to rebuild it when debugging. I thought this was simpler to just add a runtime check.

@brainlock
Copy link
Member

Ah I would have thought that carthage would make two separate builds, one when the main project is in Debug and one when it is in Release… Could you check that this isn't the case? It's weird because it would mean that when we run our project in Debug, we don't get a Debug build with symbols of our dependencies

@balland
Copy link
Author

balland commented Feb 12, 2019

When running locally, carthage is not called at each build. We just call it once as explained in https://github.com/sensational/popscan-ios/#installation

On the CI, we always start from a fresh installation as all our caching attempts failed.

@brainlock
Copy link
Member

@balland do we still need this PR? Are we still using this fork of OAuth?

@balland
Copy link
Author

balland commented Feb 5, 2021

I think we still need some fixes I did. @ajandrade could you confirm?

@ajandrade
Copy link
Member

ajandrade commented Feb 5, 2021

This PR we are not using so I would say to ignore. The fork we still need to keep.

@balland
Copy link
Author

balland commented Feb 5, 2021

If we keep the branch then this is still useful for me to be able to run the app with my dev environment without doing this tweak by hand 😄

@brainlock
Copy link
Member

If we keep the branch then this is still useful for me to be able to run the app with my dev environment without doing this tweak by hand 😄

Ah that's what this is for. I had totally forgotten in the meantime :D

Seems pretty useful, what about making it official? Anyone wanting to develop the app against a local backend will need this.

@balland
Copy link
Author

balland commented Feb 5, 2021

All my previous attempts to make a PR on the original repo were not a success but I can give it a try 😄

@ajandrade
Copy link
Member

If you want to merge it on our side, we are using our own SPM branch: https://github.com/sensational/OAuth2/tree/spm

@balland
Copy link
Author

balland commented Feb 5, 2021

As SPM should be the default now, maybe we should merge the SPM branch into master after creating a legacy branch for our older release branches that still needs carthage (in case we need to fix bugs). Anyway I guess the carthage config refers to a specific commit so the legacy branch is only need if we need to do bug fixes (very unlikely) without having the SPM changes.

@balland balland changed the base branch from master to spm January 4, 2022 13:42
@balland
Copy link
Author

balland commented Jan 4, 2022

For now I have changed the base branch to spm. @ajandrade we should maybe think of make this spm branch the new master branch. Do we still need to keep the support for carthage? If not, no need to create a legacy branch.

@balland
Copy link
Author

balland commented Jan 4, 2022

@ajandrade we should do the same for the SwiftState fork.

@brainlock brainlock removed their request for review April 14, 2022 12:07
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.

3 participants