dheerajturaga commented on code in PR #56456:
URL: https://github.com/apache/airflow/pull/56456#discussion_r2780483351
##########
airflow-core/src/airflow/provider.yaml.schema.json:
##########
@@ -31,6 +31,15 @@
"removed"
]
},
+ "build-system": {
+ "description": "Build-system of provider: Defaults to flit_core
but also can be hatchling.",
+ "type:": "string",
Review Comment:
```suggestion
"type": "string",
```
##########
dev/breeze/src/airflow_breeze/prepare_providers/provider_distributions.py:
##########
@@ -97,37 +100,60 @@ def build_provider_distribution(
f"\n[info]Building provider package: {provider_id} "
f"in format {distribution_format} in
{target_provider_root_sources_path}\n"
)
- command: list[str] = [sys.executable, "-m", "flit", "build",
"--no-setup-py", "--use-vcs"]
- get_console().print(
- "[warning]Workaround wheel-only package bug in flit by building both
and removing sdist."
- )
- # Workaround https://github.com/pypa/flit/issues/743 bug in flit that
causes .gitignored files
- # to be included in the package when --format wheel is used
- remove_sdist = False
- if distribution_format == "wheel":
- distribution_format = "both"
- remove_sdist = True
- if distribution_format != "both":
- command.extend(["--format", distribution_format])
- try:
+ provider_info = get_provider_distributions_metadata().get(provider_id)
+ if not provider_info:
+ raise RuntimeError(f"The provider {provider_id} has no provider.yaml
defined.")
+ build_backend = provider_info.get("build-system", "flit_core")
+ if build_backend == "flit_core":
+ command: list[str] = [sys.executable, "-m", "flit", "build",
"--no-setup-py", "--use-vcs"]
+ get_console().print(
+ "[warning]Workaround wheel-only package bug in flit by building
both and removing sdist."
+ )
+ # Workaround https://github.com/pypa/flit/issues/743 bug in flit that
causes .gitignored files
+ # to be included in the package when --format wheel is used
+ remove_sdist = False
+ if distribution_format == "wheel":
+ distribution_format = "both"
+ remove_sdist = True
+ if distribution_format != "both":
+ command.extend(["--format", distribution_format])
+ try:
+ run_command(
+ command,
+ check=True,
+ cwd=target_provider_root_sources_path,
+ env={
+ "SOURCE_DATE_EPOCH":
str(get_provider_details(provider_id).source_date_epoch),
+ },
+ )
+ except subprocess.CalledProcessError as ex:
+ get_console().print(f"[error]The command returned an error {ex}")
+ raise PrepareReleasePackageErrorBuildingPackageException()
+ if remove_sdist:
+ get_console().print("[warning]Removing sdist file to workaround
flit bug on wheel-only packages")
+ # Remove the sdist file if it was created
+ package_prefix = "apache_airflow_providers_" +
provider_id.replace(".", "_")
+ for file in (target_provider_root_sources_path /
"dist").glob(f"{package_prefix}*.tar.gz"):
+ get_console().print(f"[info]Removing {file} to workaround flit
bug on wheel-only packages")
+ file.unlink(missing_ok=True)
+ elif build_backend == "hatchling":
+ command = [sys.executable, "-m", "hatch", "build", "-c", "-t",
"custom"]
+ if distribution_format == "sdist" or distribution_format == "both":
+ command += ["-t", "sdist"]
+ if distribution_format == "wheel" or distribution_format == "both":
+ command += ["-t", "wheel"]
+ env_copy = os.environ.copy()
+ env_copy["SOURCE_DATE_EPOCH"] =
str(get_provider_details(provider_id).source_date_epoch)
run_command(
Review Comment:
The flit_core branch wraps run_command in a try/except CalledProcessError,
but the hatchling branch does not. If hatch build fails, the exception will
propagate with a raw traceback
##########
scripts/ci/prek/compile_provider_assets.py:
##########
@@ -117,7 +123,8 @@ def compile_assets(provider_name: str):
if try_num == 2 or INTERNAL_SERVER_ERROR not in result.stderr +
result.stdout:
print(result.stdout + "\n" + result.stderr)
sys.exit(result.returncode)
- subprocess.check_call(["yarn", "run", "build"],
cwd=os.fspath(www_directory), env=env)
+ build_cmd = ["yarn", "run"] if PROVIDERS_BUILD[provider_name] == "yarn"
else ["pnpm"]
Review Comment:
```suggestion
build_cmd = PROVIDERS_BUILD[provider_name]["build"]
```
##########
dev/breeze/src/airflow_breeze/prepare_providers/provider_distributions.py:
##########
@@ -97,37 +100,60 @@ def build_provider_distribution(
f"\n[info]Building provider package: {provider_id} "
f"in format {distribution_format} in
{target_provider_root_sources_path}\n"
)
- command: list[str] = [sys.executable, "-m", "flit", "build",
"--no-setup-py", "--use-vcs"]
- get_console().print(
- "[warning]Workaround wheel-only package bug in flit by building both
and removing sdist."
- )
- # Workaround https://github.com/pypa/flit/issues/743 bug in flit that
causes .gitignored files
- # to be included in the package when --format wheel is used
- remove_sdist = False
- if distribution_format == "wheel":
- distribution_format = "both"
- remove_sdist = True
- if distribution_format != "both":
- command.extend(["--format", distribution_format])
- try:
+ provider_info = get_provider_distributions_metadata().get(provider_id)
+ if not provider_info:
+ raise RuntimeError(f"The provider {provider_id} has no provider.yaml
defined.")
+ build_backend = provider_info.get("build-system", "flit_core")
+ if build_backend == "flit_core":
+ command: list[str] = [sys.executable, "-m", "flit", "build",
"--no-setup-py", "--use-vcs"]
+ get_console().print(
+ "[warning]Workaround wheel-only package bug in flit by building
both and removing sdist."
+ )
+ # Workaround https://github.com/pypa/flit/issues/743 bug in flit that
causes .gitignored files
+ # to be included in the package when --format wheel is used
+ remove_sdist = False
+ if distribution_format == "wheel":
+ distribution_format = "both"
+ remove_sdist = True
+ if distribution_format != "both":
+ command.extend(["--format", distribution_format])
+ try:
+ run_command(
+ command,
+ check=True,
+ cwd=target_provider_root_sources_path,
+ env={
+ "SOURCE_DATE_EPOCH":
str(get_provider_details(provider_id).source_date_epoch),
+ },
+ )
+ except subprocess.CalledProcessError as ex:
+ get_console().print(f"[error]The command returned an error {ex}")
+ raise PrepareReleasePackageErrorBuildingPackageException()
+ if remove_sdist:
+ get_console().print("[warning]Removing sdist file to workaround
flit bug on wheel-only packages")
+ # Remove the sdist file if it was created
+ package_prefix = "apache_airflow_providers_" +
provider_id.replace(".", "_")
+ for file in (target_provider_root_sources_path /
"dist").glob(f"{package_prefix}*.tar.gz"):
+ get_console().print(f"[info]Removing {file} to workaround flit
bug on wheel-only packages")
+ file.unlink(missing_ok=True)
+ elif build_backend == "hatchling":
+ command = [sys.executable, "-m", "hatch", "build", "-c", "-t",
"custom"]
+ if distribution_format == "sdist" or distribution_format == "both":
+ command += ["-t", "sdist"]
+ if distribution_format == "wheel" or distribution_format == "both":
+ command += ["-t", "wheel"]
+ env_copy = os.environ.copy()
Review Comment:
hatchling takes full env here. This seems inconsistent from the above?
##########
scripts/ci/prek/compile_provider_assets.py:
##########
@@ -56,6 +56,10 @@
"hash": PROVIDERS_ROOT / "edge3" / "www-hash.txt",
},
}
+PROVIDERS_BUILD = {
+ "fab": "yarn",
+ "edge": "pnpm",
+}
Review Comment:
nit, Make PROVIDERS_BUILD encode the full commands instead of just the
package manager name, so there's no inline conditional logic ?
```suggestion
PROVIDERS_BUILD = {
"fab": {
"install": ["yarn", "install", "--frozen-lockfile"],
"build": ["yarn", "run", "build"],
},
"edge": {
"install": ["pnpm", "install", "--frozen-lockfile"],
"build": ["pnpm", "build"],
},
}
```
##########
scripts/ci/prek/compile_provider_assets.py:
##########
@@ -104,9 +108,11 @@ def compile_assets(provider_name: str):
env = os.environ.copy()
env["FORCE_COLOR"] = "true"
for try_num in range(3):
- print(f"### Trying to install yarn dependencies: attempt: {try_num +
1} ###")
+ print(
+ f"### Trying to install {PROVIDERS_BUILD[provider_name]}
dependencies: attempt: {try_num + 1} ###"
+ )
result = subprocess.run(
- ["yarn", "install", "--frozen-lockfile"],
+ [PROVIDERS_BUILD[provider_name], "install", "--frozen-lockfile"],
Review Comment:
```suggestion
PROVIDERS_BUILD[provider_name]["install"],
```
##########
dev/breeze/src/airflow_breeze/utils/run_utils.py:
##########
@@ -508,6 +508,7 @@ def run_compile_ui_assets(
dev: bool,
run_in_background: bool,
force_clean: bool,
+ additional_ui_hooks: list[str],
Review Comment:
The additional_ui_hooks parameter is added as a required positional
parameter with default. Looks like not all callers are updated. Consider adding
a default value `additional_ui_hooks: list[str] = []` ?
https://github.com/apache/airflow/blob/bfaf4ffc4881fb21e8303bb9cf07d22a9e226a9d/dev/breeze/src/airflow_breeze/commands/ui_commands.py#L713
```suggestion
additional_ui_hooks: list[str] = [],
```
##########
dev/breeze/src/airflow_breeze/prepare_providers/provider_distributions.py:
##########
@@ -97,37 +100,60 @@ def build_provider_distribution(
f"\n[info]Building provider package: {provider_id} "
f"in format {distribution_format} in
{target_provider_root_sources_path}\n"
)
- command: list[str] = [sys.executable, "-m", "flit", "build",
"--no-setup-py", "--use-vcs"]
- get_console().print(
- "[warning]Workaround wheel-only package bug in flit by building both
and removing sdist."
- )
- # Workaround https://github.com/pypa/flit/issues/743 bug in flit that
causes .gitignored files
- # to be included in the package when --format wheel is used
- remove_sdist = False
- if distribution_format == "wheel":
- distribution_format = "both"
- remove_sdist = True
- if distribution_format != "both":
- command.extend(["--format", distribution_format])
- try:
+ provider_info = get_provider_distributions_metadata().get(provider_id)
+ if not provider_info:
+ raise RuntimeError(f"The provider {provider_id} has no provider.yaml
defined.")
+ build_backend = provider_info.get("build-system", "flit_core")
+ if build_backend == "flit_core":
+ command: list[str] = [sys.executable, "-m", "flit", "build",
"--no-setup-py", "--use-vcs"]
+ get_console().print(
+ "[warning]Workaround wheel-only package bug in flit by building
both and removing sdist."
+ )
+ # Workaround https://github.com/pypa/flit/issues/743 bug in flit that
causes .gitignored files
+ # to be included in the package when --format wheel is used
+ remove_sdist = False
+ if distribution_format == "wheel":
+ distribution_format = "both"
+ remove_sdist = True
+ if distribution_format != "both":
+ command.extend(["--format", distribution_format])
+ try:
+ run_command(
+ command,
+ check=True,
+ cwd=target_provider_root_sources_path,
+ env={
+ "SOURCE_DATE_EPOCH":
str(get_provider_details(provider_id).source_date_epoch),
Review Comment:
flit_core, env is set to only SOURCE_DATE_EPOCH (wiping all other env vars),
while for hatchling, os.environ.copy() is used and SOURCE_DATE_EPOCH is added.
the inconsistency is worth noting since the flit path may break if flit needs
PATH or other env vars
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
