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

Add SPDX license id lines to source files #618

Conversation

jafingerhut
Copy link
Contributor

No description provided.

@jafingerhut jafingerhut marked this pull request as draft February 2, 2025 04:16
@jafingerhut
Copy link
Contributor Author

Putting this in Draft status right now, as there are still a few more source files I have not added license id lines to yet.

@jafingerhut
Copy link
Contributor Author

I know nothing about Bazel except that it is a build tool, or something in that neighborhood.

Does anyone have any thoughts on why adding some comment lines to some source files would cause the Bazel build to fail, when it doesn't fail if all I do is make a small comment change in one of the BUILD files, as shown in this PR created only for the purpose of seeing if the current files, or something closer to them, passes the Bazel CI build check? #619

@antoninbas
Copy link
Member

@jafingerhut The Bazel CI job uses a cache, which is persisted across jobs.

My guess is that the build issue you are observing for this PR is a legitimate issue, which got exposed by touching all source files and invalidating the cache. In contrast, your test PR (#619) does not exhibit the issue because if you look at the logs for the job, you will see that the job took under a minute and simply got its outputs from the Bazel cache.

I suspect that if you make a single line change to one of the files listed in the Bazel error for this PR (e.g., lib/vector.c), you may also get a build error.
The errors look legitimate to me, and they may not be too hard to fix if it's just some missing Bazel dependencies.

@jafingerhut
Copy link
Contributor Author

@smolkaj Do you know if anyone at Google is using the code in this repository with Bazel for building it? I am about 1% knowledgeable of Bazel, and from Antonin's comment above, it appears like the Bazel CI build in this repo might not be configured properly via its BUILD files right now. I am far from the best person to try to fix it.

@smolkaj
Copy link
Member

smolkaj commented Feb 3, 2025

Catching up after returning from parental leave today...

We seem to currently use our own, separate BUILD rules at Google internally.

@jafingerhut
Copy link
Contributor Author

By pinning the Bazel CI run to Ubuntu 22.04, it passes again. See this comment: #620 (comment)

@jafingerhut jafingerhut marked this pull request as ready for review February 4, 2025 14:02
@jafingerhut
Copy link
Contributor Author

I'm removing draft status on this PR. It has the one-line CI change so that the Bazel build CI test runs on Ubuntu 22.04, which seems like a reasonable change to make until/unless someone updates the Bazel build stuff to work on Ubuntu 24.04.

@jafingerhut
Copy link
Contributor Author

@antoninbas Sorry to trouble you for such a repetitive PR. 95% of it is adding one line with a comment containing "SPDX-License-Identifier: Apache-2.0" to a file that already has a copyright notice and the couple of paragraphs saying it has an Apache 2.0 license.

4.9% of it is adding that line plus a copyright notice to files that had not copyright notice, mainly a few Bash scripts. I checked the commit logs of those and you were the only committer on all of them I can remember, going back to 2016-2017 or so.

0.1% of it is changing the CI so it only uses ubuntu-22.04, rather than ubuntu-latest, which changed from 22.04 to 24.04 a couple of months ago and I'm pretty sure is the reason the Bazel build was failing. A similar thing happened in the p4runtime repo a couple of months back, and we used the same temporary fix until the Bazel build files were updated to work with Ubuntu 24.04, which I hope happens to this repo soon by someone who knows more about Bazel than I do.

@jafingerhut
Copy link
Contributor Author

FYI, I am working on a simple Python program that we can use later in CI to verify that all source files have an SPDX-License-Identifier line, but that will be later after we get more p4lang repos "clean" according to it: https://github.com/jafingerhut/test-spdx-checker

@@ -10,7 +10,7 @@ on:

jobs:
build:
runs-on: ubuntu-latest
runs-on: ubuntu-22.04
Copy link
Member

Choose a reason for hiding this comment

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

haven't you already taken care of this in #620?
could you rebase this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I went ahead and threw it in this PR as well, to see if CI passed or not. I will rebase.

…ub.com:jafingerhut/PI into add-spdx-license-identifiers-to-source-files-1
@jafingerhut
Copy link
Contributor Author

Got Antonin's approval, rebased, all CI tests pass as expected. I will go ahead and hit the merge button on this one.

@jafingerhut jafingerhut merged commit 17802cf into p4lang:main Feb 5, 2025
6 checks passed
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