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

reloc: add arch to relocatable package filename #8709

Closed

Conversation

syuu1228
Copy link
Contributor

To avoid dequeue, just merging this PR is not enough, I think we need to do as follows:

  • support $(arch).tar.gz on scylla-ccm
  • support $(arch).tar.gz on jenkins-pipeline scripts
  • merge renaming patch for scylla-tools / scylla-jmx / scylla-python3 first
  • add submodule update on this PR

Add architecture name for relocatable packages, to support distributing
both x86_64 version and aarch64 version.

Fixes #8675

@syuu1228
Copy link
Contributor Author

syuu1228 commented May 26, 2021

Submodule part of this PR are:
scylladb/scylla-python3#19

But as I wrote on above, I think we need to modify ccm and jenkins script first.

@syuu1228 syuu1228 requested a review from hagitsegev May 26, 2021 11:06
@syuu1228
Copy link
Contributor Author

Ah, it may also affect nonroot installer test, but I'm not sure about it.

@syuu1228
Copy link
Contributor Author

@hagitsegev Could you check it this what we want, and also can you add support new filename on jenkins pipeline?

Copy link
Contributor

@hagitsegev hagitsegev left a comment

Choose a reason for hiding this comment

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

I think that the name structure of .arch is a bit strange. Looking on
msys2/MINGW-packages#4787 I see they use dash:
package naming convention: "${package name}-${build mode not on release}-${optional architecture}-package.tar.gz"
But I'm not the one to decide.

I'm setting the pipelines to be agnostic for name with or without the architecture (we should agree about the convention). So we will not have to coordinate this merge with mine. https://github.com/scylladb/scylla-pkg/pull/2100

configure.py Outdated
@@ -1983,18 +1985,18 @@ def configure_abseil(build_dir, mode, mode_config):
f.write(textwrap.dedent('''\
build $builddir/{mode}/iotune: copy $builddir/{mode}/seastar/apps/iotune/iotune
''').format(**locals()))
f.write('build $builddir/{mode}/dist/tar/{scylla_product}-package.tar.gz: package $builddir/{mode}/scylla $builddir/{mode}/iotune $builddir/SCYLLA-RELEASE-FILE $builddir/SCYLLA-VERSION-FILE $builddir/debian/debian $builddir/node_exporter | always\n'.format(**locals()))
f.write('build $builddir/{mode}/dist/tar/{scylla_product}-package.{arch}.tar.gz: package $builddir/{mode}/scylla $builddir/{mode}/iotune $builddir/SCYLLA-RELEASE-FILE $builddir/SCYLLA-VERSION-FILE $builddir/debian/debian $builddir/node_exporter | always\n'.format(**locals()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not having some variable to take "$builddir/{mode}/dist/tar/{scylla_product}"? It appears 38 times.
Also the 2nd part of "package.{arch}.tar.gz"

@asias
Copy link
Contributor

asias commented May 31, 2021

To keep compatibility, can we leave the old package name as a link to the new package? It is hard to maintain a build script which has to handle 3+ different URL for the package. Also, most devs is on X86 anyway.

@penberg
Copy link
Contributor

penberg commented May 31, 2021

@hagitsegev I'm fine with your proposed naming convention. @syuu1228 any objections to switching over to using a dash?

@avikivity
Copy link
Member

Using $arch in the package name is not mandatory, we could publish it in a subdirectory $arch/scylla-unified....tar.gz.

However, I think it's a good idea to include $arch in the package name, it saves confusion later and it is what other package formats (rpm/deb) do.

@avikivity
Copy link
Member

To keep compatibility, can we leave the old package name as a link to the new package? It is hard to maintain a build script which has to handle 3+ different URL for the package. Also, most devs is on X86 anyway.

Good idea.

@syuu1228 syuu1228 force-pushed the add_arch_to_relocatable_package branch from 1e6d256 to 82f58a8 Compare June 9, 2021 13:08
@syuu1228
Copy link
Contributor Author

syuu1228 commented Jun 9, 2021

Changed package name structure to "${package name}-${build mode not on release}-${optional architecture}-package.tar.gz".
Also added symlink to older filename to keep compatibility.

@syuu1228 syuu1228 changed the title [DO NOT MERGE YET] reloc: rename relocatable package to <package name>.<arch>.tar.gz [DO NOT MERGE YET] reloc: add arch to relocatable package filename Jun 9, 2021
Copy link
Contributor

@hagitsegev hagitsegev left a comment

Choose a reason for hiding this comment

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

LGTM
I just need to see how the sym link looks like.
Maybe I need to add it to pkg somehow?

@syuu1228
Copy link
Contributor Author

LGTM
I just need to see how the sym link looks like.
Maybe I need to add it to pkg somehow?

Here's how does it looks like:

[syuu@monster scylla.6]$ ls -l build/release/dist/tar/
total 1149636
lrwxrwxrwx. 1 syuu docker        32 Jun  9 21:51 scylla-jmx-package.tar.gz -> scylla-jmx-x86_64-package.tar.gz
-rw-r--r--. 1 syuu docker   8662194 Jun  9 21:31 scylla-jmx-x86_64-package.tar.gz
lrwxrwxrwx. 1 syuu docker        28 Jun  9 21:57 scylla-package.tar.gz -> scylla-x86_64-package.tar.gz
lrwxrwxrwx. 1 syuu docker        36 Jun  9 21:51 scylla-python3-package.tar.gz -> scylla-python3-x86_64-package.tar.gz
-rw-r--r--. 1 syuu docker  24034104 Jun  9 21:31 scylla-python3-x86_64-package.tar.gz
lrwxrwxrwx. 1 syuu docker        34 Jun  9 21:51 scylla-tools-package.tar.gz -> scylla-tools-x86_64-package.tar.gz
-rw-r--r--. 1 syuu docker  33885343 Jun  9 21:31 scylla-tools-x86_64-package.tar.gz
lrwxrwxrwx. 1 syuu docker        65 Jun  9 21:57 scylla-unified-package-4.6.dev.0.20210607.1e6d25614.tar.gz -> scylla-unified-x86_64-package-4.6.dev.0.20210607.1e6d25614.tar.gz
-rw-r--r--. 1 syuu docker 588092293 Jun  9 21:57 scylla-unified-x86_64-package-4.6.dev.0.20210607.1e6d25614.tar.gz
-rw-r--r--. 3 syuu docker 522545302 Jun  9 21:57 scylla-x86_64-package.tar.gz

@syuu1228 syuu1228 changed the title [DO NOT MERGE YET] reloc: add arch to relocatable package filename reloc: add arch to relocatable package filename Jun 14, 2021
@syuu1228
Copy link
Contributor Author

ping

@hagitsegev
Copy link
Contributor

hagitsegev commented Jun 17, 2021

Sorry for the delay.
As for the links - we don't need a link for the "all archs" packages, such as JMX and tools. No change in their names.
We don't need to change PKG, as it is currently agnostic to name with or without architecture.
On could (S3) - we can't put links there. Pkg code is currently agnostic to name with or without architecture.

I don't have a good way to verify that everything works after we merge this. But I think we can go ahead, and I'll fix if there will a need.

@syuu1228
Copy link
Contributor Author

syuu1228 commented Jun 21, 2021

As for the links - we don't need a link for the "all archs" packages, such as JMX and tools. No change in their names.

Ah, you are right.

On could (S3) - we can't put links there. Pkg code is currently agnostic to name with or without architecture.

How about just copy file instead of symlink?

@avikivity
Copy link
Member

Copying the same huge file twice increases the build time, because s3 upload is so slow.

If all the tests are ready, perhaps we can copy just the new file name.

@syuu1228 syuu1228 force-pushed the add_arch_to_relocatable_package branch from 82f58a8 to 04d0a40 Compare June 21, 2021 10:14
@syuu1228
Copy link
Contributor Author

Copying the same huge file twice increases the build time, because s3 upload is so slow.

If all the tests are ready, perhaps we can copy just the new file name.

Okay, S3 upload itself is done in Jenkins script, not build script so we don't need to do anything here.
But I think it's better to keep copying packages to older name, it may prevent breaking Jenkins build and prevent dequeueing.
After we make sure everything works with new package name, we can drop the copying.

Add architecture name for relocatable packages, to support distributing
both x86_64 version and aarch64 version.

Also create symlink from new filename to old filename to keep
compatibility with older scripts.

Fixes scylladb#8675
@syuu1228 syuu1228 force-pushed the add_arch_to_relocatable_package branch from 04d0a40 to db62fb6 Compare June 21, 2021 11:13
@syuu1228
Copy link
Contributor Author

@hagitsegev Stopped renaming scylla-jmx and scylla-tools-java, replace symlink with copy rule.

@hagitsegev
Copy link
Contributor

hagitsegev commented Jun 21, 2021

Copying the same huge file twice increases the build time, because s3 upload is so slow.
If all the tests are ready, perhaps we can copy just the new file name.

Okay, S3 upload itself is done in Jenkins script, not build script so we don't need to do anything here.
But I think it's better to keep copying packages to older name, it may prevent breaking Jenkins build and prevent dequeueing.
After we make sure everything works with new package name, we can drop the copying.

I set the pipeline to be agnostic to the name.
This is also a "temp" solution till this feature is stable.
I look for the new name, and if can't find - for the old, and only if both can't be found - I give an error.

avikivity pushed a commit that referenced this pull request Jun 21, 2021
Add architecture name for relocatable packages, to support distributing
both x86_64 version and aarch64 version.

Also create symlink from new filename to old filename to keep
compatibility with older scripts.

Fixes #8675

Closes #8709
@avikivity
Copy link
Member

Was dequeued (by @psarna), causes dist-python3-rpm to fail.

@syuu1228
Copy link
Contributor Author

@avikivity Could you please merge scylladb/scylla-python3#19, sync submodule then merge this PR again?
Seems like the build was failing at scylla-python3 because the PR doesn't applied.

@syuu1228
Copy link
Contributor Author

Note: failed jenkins job was seems like https://jenkins.scylladb.com/view/nexts/job/scylla-master/job/next/3675,
Saying scylla-python3-x86_64-package.tar.gz does not exist, which means it invoked build_reloc.sh but scylla-python3-x86_64-package.tar.gz did not appeared. Because target filename is still scylla-python3-package.tar.gz, since it didn't merged the PR.

22:12:02  FAILED: dist-python3-rpm 
22:12:02  cd tools/python3 && ./reloc/build_rpm.sh --reloc-pkg build/scylla-python3-x86_64-package.tar.gz
22:12:02  build/scylla-python3-x86_64-package.tar.gz does not exist.

lauranovich pushed a commit to lauranovich/scylla that referenced this pull request Jul 29, 2021
Add architecture name for relocatable packages, to support distributing
both x86_64 version and aarch64 version.

Also create symlink from new filename to old filename to keep
compatibility with older scripts.

Fixes scylladb#8675

Closes scylladb#8709

[update tools/python3 submodule:

* tools/python3 ad04e8e...afe2e7f (1):
  > reloc: add arch to relocatable package filename
]
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.

Ninja build feature request: Set packages to be created with architecture name
5 participants