Skip to content

Changes to onboarding process#6560

Merged
kbighorse merged 10 commits intomainfrom
kbighorse/onboarding
Aug 9, 2022
Merged

Changes to onboarding process#6560
kbighorse merged 10 commits intomainfrom
kbighorse/onboarding

Conversation

@kbighorse
Copy link
Contributor

No description provided.

Comment on lines 58 to -59
run "gem install thin -v 1.5.1 -- --with-cflags=\"-Wno-error=implicit-function-declaration\""
run "gem install mailcatcher -- --with-cppflags=-I/usr/local/opt/openssl/include"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we're always running into issues with new folks installing things with the right flags, is there a way to keep these flag instructions in the Gemfile? slack link

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two questions about this:

  1. Is it required that (mac) users install openssl via brew? I assume this is because of the lack of header files in the default install if so
  2. Homebrew has changed its directory structure for M1 macs (learned this yesterday). Perhaps we can just case on the system information and provide the correct flag in each case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I am not sure! I feel it's one of those "just copy whatever somebody else said to make it work" kinda situations for me
  2. Yes, M1 Macs are going to have to do things differently, my computer is x86 so I have no firsthand experience with this yet 😬

But don't let that stop cleaning this up! I do think it's better to have these in the Gemfile than this script

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for moving to Gemfile. I did hit dependency errors though with all recent versions of mailcatcher :/. (Both Eric and I use M1s)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this stems from the author recommending against installing Mailcatcher via bundler: https://mailcatcher.me/#bundler

Conflicting versions strikes me as a potential problem, but I'd be fine with it as long as there are no conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pauldoomgov, @mitchellhenke mentioned you might have a M1 too? Have you run into this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitchellhenke you're right, I forgot about that. I introduced that problem, even though we still have the original problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an observation: the versions of mailcatcher and thin that this installs are from 2011 and 2013, respectively. I think I'm going to try and get current versions working and keep it out of bundler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even need thin ? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think mailcatcher needs thin... which gives me another idea 💡 what if we had Gemfile.mailcatcher and updated our scripts to use that completely separately?

BUNDLE_GEMFILE=Gemfile.mailcatcher bundle exec mailcatcher
# Gemfile.mailcatcher

gem 'mailcatcher'

darth-cheney and others added 6 commits July 8, 2022 11:27
-- Notes
For mailcatcher to install correctly on macOS (M1 and Intel), we need
to install it via brew and link to the correct header location. The
instructions specify how to do that before installing.
include note for openssl issue we resolved
-- Notes
We have two concurrent problems with openssl:
First, on M1 macs, openssl's libraries are in a different location.
Second -- and more relevant to this particular commit -- eventmachine
is not able to properly use its C extensions with openssl@1.3, which
is what homebrew will install by default. This will fail silently
during the setup/install phase and only become apparent when
attempting to run the application.

The solution is to strictly use openssl@1.1 for the moment.
-- What
1. `yarn build` was not being called before tests in the Makefile. So
if you pulled down the repository, ran `make setup` and followed all
the instructions in the readme, tests would still fail because at no
point in that process do js bundles get built. I have added this to
the relevant Makefile actions
2. The README had a typo about openssl1.3 when we require 1.1

changelog: Improvements, Documentation, Updating onboarding
documentation (LG-6972)
@kbighorse kbighorse marked this pull request as ready for review July 27, 2022 17:16
@kbighorse kbighorse requested a review from solipet July 27, 2022 17:16
Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me, although I don't have an M1 MBP so I can't test them out.

@kbighorse kbighorse merged commit ef49bf1 into main Aug 9, 2022
@kbighorse kbighorse deleted the kbighorse/onboarding branch August 9, 2022 18:08
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.

7 participants