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

Reorganize the source tree #443

Merged
merged 15 commits into from
Nov 4, 2021
Merged

Reorganize the source tree #443

merged 15 commits into from
Nov 4, 2021

Conversation

aiuto
Copy link
Collaborator

@aiuto aiuto commented Oct 21, 2021

  • WORKSPACE moves up one level
    • this makes //:pkg.bzl effectively //pkg:pkg.bzl
    • put forwarders in place at top level to not break anyone
  • tests moves up one level
  • toolchains moves up one level
  • examples/**/BUILD do not change to the new rule locations
    • this is explicitly to provide some proof that the forwarders work
    • new examples should use the proper paths to .bzl files
    • near the 1.x release we can clean the current ones up

Updates the version to 0.6.0. It is mostly an invisible change, but some usage around cloning/vendoring might slightly break.

Fixes: #104

@aiuto aiuto requested a review from nacl as a code owner October 21, 2021 18:41
- WORKSPACE moves up one level
  - this makes //:pkg.bzl effectively //pkg:pkg.bzl
  - put forwarders in place at top level to not break anyone
- tests moves up one level
- toolchains moves up one level
- examples/**/BUILD do not change to the new rule locations
  - this is explicitly to provide some proof that the forwarders work
  - new examples should use the proper paths to .bzl files
  - near the 1.x release we can clean the current ones up

Fixes: bazelbuild#111
@aiuto aiuto marked this pull request as draft October 21, 2021 20:06
@aiuto aiuto marked this pull request as ready for review October 21, 2021 21:42
@aiuto aiuto added this to the 1.0 milestone Oct 22, 2021
@aiuto aiuto mentioned this pull request Oct 24, 2021
@nacl
Copy link
Collaborator

nacl commented Nov 3, 2021

I'll take a deeper look shortly, but one quick thing that comes to mind is that while this won't break anyone using the release archive, it will break people directly cloning the git repo. Is this intentional? Seems fine to me.

@aiuto
Copy link
Collaborator Author

aiuto commented Nov 3, 2021 via email

@nacl
Copy link
Collaborator

nacl commented Nov 3, 2021

I am fine with that. I don't think it will break that many people, because
they old workspace is still around,

That's fine, but looking at https://github.com/aiuto/rules_pkg/tree/reroot/pkg, the old WORKSPACE seems to be missing.

Copy link
Collaborator

@nacl nacl left a comment

Choose a reason for hiding this comment

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

Cool. Most of the stuff I saw here was nits that buildifier might see later. Nothing critical.

deps.bzl Show resolved Hide resolved
package_variables.bzl Outdated Show resolved Hide resolved
providers.bzl Outdated Show resolved Hide resolved
tests/mappings/filter_directory/BUILD Outdated Show resolved Hide resolved
version.bzl Show resolved Hide resolved
tests/rpm/analysis_tests.bzl Outdated Show resolved Hide resolved
tests/rpm/BUILD Outdated Show resolved Hide resolved
tests/mappings/mappings_test.bzl Show resolved Hide resolved
tests/mappings/mappings_external_repo_test.bzl Outdated Show resolved Hide resolved
tests/install/BUILD Outdated Show resolved Hide resolved
@aiuto
Copy link
Collaborator Author

aiuto commented Nov 3, 2021 via email

@aiuto aiuto merged commit 3c520a1 into bazelbuild:main Nov 4, 2021
@aiuto aiuto deleted the reroot branch November 4, 2021 02:44
nacl pushed a commit to nacl/rules_pkg that referenced this pull request Nov 4, 2021
As of bazelbuild#443, all of the rules_pkg files are now in a WORKSPACE relative to the
root of the repository.  These are now shared with the `deb_packages/` and
`examples/` subdirectories, which contain their own WORKSPACE files.  This
confuses Bazel and prevents us from testing `//...`.

This changes allows us to build/test `//...` by adding those directories to a
.bazelignore file at the root of the repository.

Most people working on rules_pkg aren't going to be touching these, and if they
are, they need to be in those WORKSPACEs anyway, so there should be no harm in
this.
aiuto pushed a commit that referenced this pull request Nov 4, 2021
As of #443, all of the rules_pkg files are now in a WORKSPACE relative to the
root of the repository.  These are now shared with the `deb_packages/` and
`examples/` subdirectories, which contain their own WORKSPACE files.  This
confuses Bazel and prevents us from testing `//...`.

This changes allows us to build/test `//...` by adding those directories to a
.bazelignore file at the root of the repository.

Most people working on rules_pkg aren't going to be touching these, and if they
are, they need to be in those WORKSPACEs anyway, so there should be no harm in
this.
nacl pushed a commit to nacl/rules_pkg that referenced this pull request Nov 9, 2021
bazelbuild#443 reorganized the source tree: moving the WORKSPACE to the root of the
repository as is convention.  To help maintain backward compatibility, numerous
stub files were added to the root of the repository.

It looks like the stub for `install.bzl` wasn't completely done and slipped
through the cracks in the review.  This change rectifies the issue.
nacl pushed a commit that referenced this pull request Nov 11, 2021
#443 reorganized the source tree: moving the WORKSPACE to the root of the
repository as is convention.  To help maintain backward compatibility, numerous
stub files were added to the root of the repository.

I noticed the following in local testing:

- The `install.bzl` stub is empty.
- The `mappings.bzl` stub is missing `REMOVE_BASE_DIRECTORY`
- The renaming of the `deps` value in the `pkg_install` macro confuses bazel; force it to use a rules_pkg-relative label instead (with the `Label` constructor) 

These slipped through the cracks in the review.  This change rectifies the above issues.
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.

Release assets don't match repo structure
2 participants