https://linen.dev logo
j

Jenny Brown

08/03/2021, 10:23 PM
Airbyte team - I’m running on a brand new clean branch.  I run ./gradlew format and there are whitespace changes in files.  I didn’t expect that to be possible with the build-enforced whitespace formatter.  It affects 4 different connectors, and hit java, json, and py files. Is this a side effect of not automatically running the connector build when building platform? Perhaps we need to see if we can still force the whitespace format, even without doing a full build?
u

user

08/03/2021, 10:25 PM
weird. what's the commit hash?
j

Jenny Brown

08/03/2021, 10:27 PM
21e6d49d, b7c8101ca, 25c04db29, 9e529545c2 Those seem to be the hashes of some of the affected files. There might be a few more hashes; I didn’t verify every file. These are on master with bad whitespace at the moment.
u

user

08/03/2021, 10:54 PM
the behavior you're seeing is definitely wrong. that being said. is there a reason why you're using that command as opposed to:
Copy code
SUB_BUILD=PLATFORM ./gradlew format
this will only format the platform part of the project which makes life a lot easier because you don't need to install all the connector python depedencies.
u

user

08/03/2021, 11:00 PM
sorry. i know that's not super helpful, but i can't realistically get all the connectors to install all of their deps without running into a failure.
u

user

08/03/2021, 11:01 PM
is this what you're seeing?
u

user

08/03/2021, 11:01 PM
this is what i see when format fails somewhere in the middle.
u

user

08/03/2021, 11:05 PM
Yeah that’s the same set of files. I’m working on a connector build right now… I decided to try to make sure whitespace was not-wrong before I started, and apparently targeted too broad of an area for it, but that worked fine in the past so I didn’t realize a change in process was necessary. I’m mainly trying to reformat my own connector json and java for now.
u

user

08/03/2021, 11:07 PM
got it.
j

Jenny Brown

08/03/2021, 11:08 PM
@s do you have advice on how to run format on a single connector?
u

user

08/03/2021, 11:08 PM
I actually don't really know how to do this.
u

user

08/03/2021, 11:16 PM
is the format command failing for you jenny?
u

user

08/03/2021, 11:17 PM
typically if format fails halfway through then you might find some half-formatted files
u

user

08/03/2021, 11:17 PM
Well, it failed when I had a bad json in there, but I moved my bad json out of the way and then ran format again, and was still seeing whitespace differences even from a pristine state.
j

Jenny Brown

08/03/2021, 11:18 PM
I suspect the reason this is not reflected in the master branch is because of this line: https://github.com/airbytehq/airbyte/blob/master/.github/workflows/gradle.yml#L100 it’s running on connectors base not all connectors which means there could be formatting changes not checked into git. So running
./gradlew build
locally (which formats) could produce local diffs
u

user

08/03/2021, 11:19 PM
charles, was it intentional that this is formatting connectors_base and not connectors?
u

user

08/03/2021, 11:19 PM
my guess is if we change that line to
=CONNECTORS
then it’ll fail master build
u

user

08/03/2021, 11:25 PM
Do we need a ticket on this? Is one of you doing a fix, or should I make a PR?
u

user

08/03/2021, 11:28 PM
errr. yeah. some combination of oversight and just not knowing how formatting should work for connectors.
u

user

08/03/2021, 11:29 PM
i generally think that any command that requires running pip install for every single connector is not going to go well. but probably getting this right in the build now is wortwhile and then evolving from there.
u

user

08/03/2021, 11:33 PM
Maybe each python connector should have its own pyenv or conda environment. That would at least narrow the scope of conflicts. But yes that’s for later.
j

Jenny Brown

08/03/2021, 11:34 PM
haha. that's actually the problem (to some extent). every python connector has its own pyenv which means that we spend a lot of time reinstalling the same deps over and over.
u

user

08/03/2021, 11:35 PM
A prewarmed local cache might help speed, but yeah, I think that’s largely inevitable. As long as they’re based on differing libraries, they’re truly each their own “build”, and that’s the nature of a build artifact. It’s just a pain for this kind of scenario. Every workaround I know of comes with its own downsides.
u

user

08/03/2021, 11:36 PM
agreed.
u

user

08/03/2021, 11:39 PM
I’ll pick back up in the morning. Let me know if I need to do anything more complex than ‘git pull; git rebase master’ to pick up a fix. I’m not sure who’s actually doing the fix for this.
3 Views