remove non-OSI license code to ensure it's not used in builds #163
Conversation
| @@ -53,4 +53,7 @@ pushd "${VAULT_FOLDER}" | |||
| git fetch --tags --depth 1 origin "${VAULT_VERSION}" | |||
| # Checkout the branch we want | |||
| git -c advice.detachedHead=false checkout FETCH_HEAD | |||
| # Make sure we don't use non-OSI code | |||
| # requires patching in next step | |||
| rm -rf bitwarden_license | |||
There was a problem hiding this comment.
This doesn't seem useful, that directory is not packed or used, only pulled as part of the repo. Removing this doesn't serve anything useful.
| "files": [ | ||
| "src/polyfills.ts", | ||
| "src/main.ts", | ||
| - "../../bitwarden_license/bit-web/src/main.ts", |
There was a problem hiding this comment.
This isn't needed anymore as this is already changed upstream https://github.com/bitwarden/clients/pull/8818/files
So, I also do not see the huge benefit here either.
We already build the OSS version, which is determined by Bitwarden to be GPLv3 compatibel ans not using there Bitwarden Licensed code.
There was a problem hiding this comment.
I think the benefit would be the assurance that the compiled code is actually GPLv3 compatible and not relying on the content in the bitwarden_license directory at all to be built, which might not always be the case. (E.g. in web-v2023.8.0 to web-v2023.9.2 the secrets manager logo was technically included as part of the compilation when it was located in the bitwarden_license directory)
It also enables third party vendors to be able to ship the source code without bundling the non-free code into their packages as explained in #162. (If we were to create a soft fork, we could also easily remove the non-free code from our repository to be on the safe side.)
There was a problem hiding this comment.
Precisely. For distros and probably others all assurances should be taken that no non-OSI code ends up in the distro by mistake. That said, it's standard practice to "rm -rf" anything you want to make sure isn't included or built against in RPM spec files.
My intent is to use the published/built sources for web vault since it's not binary, which means I can't rm -rf at build time with a Fedora-specific patch.
Since the patch does no harm and removes what's not being used anyway, it addresses the concern by having multiple things that would have to fail for the code to get into the build by mistake, while having 0 impact on existing functionality/expectations.
There was a problem hiding this comment.
I'm for the soft fork option but not currently in this repo. Though i still wonder if it isn't more work with upstream rebases etc. (But that is something i haven't looked at actually)
In general I agree that if possible without too much hassle to try and keep clear of any possible GPLv3 invalid build.
But seeing that upstream already made some changes regarding this i rather wait for a new release with those changes incorporated.
There was a problem hiding this comment.
Personally I would remove the bitwarden_license references from all files for good measure but it should build just fine with only removing it from the apps/web/tsconfig.json.
Not sure if we really want to remove the bitwarden_license directory though as it would require additional handling in the scripts.
| # Make sure we don't use non-OSI code | ||
| # requires patching in next step | ||
| rm -rf bitwarden_license |
There was a problem hiding this comment.
This should probably be done in the apply_patches.sh script so it will also be removed from the Docker build.
Also if you remove this directory you should also exclude it from the generated patch file:
bw_web_builds/scripts/generate_patch_file.sh
Lines 40 to 41 in cc03d26
There was a problem hiding this comment.
I think it's too much for something not used at all.
| - "../../bitwarden_license/bit-web/src/main.ts", | ||
| "src/theme.ts" | ||
| ], | ||
| "include": [ |
There was a problem hiding this comment.
If I try to apply this patch file it will complain because of the line ending. If you generate the patch file with make generate-patch this should be corrected.
Which is already done upstream, so i do not see a use case for this until a new released version of the web-vault, at which point it will be solved automatically.
It will only complicate stuff with the current way of patching i think. |
|
Btw, if it makes releasing your package really more useful in the short term. Then i do not have any strong feelings against it. But if not, then i would rather wait for a new release to at least add the directory removal. |
|
Closing as fixed via #169 |
fixes #162