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

R-bundle-CRAN-2023b, Brunsli-0.1 patch, and easyconfigs.py #20756

Closed
wants to merge 23 commits into from

Conversation

moravveji
Copy link

@moravveji moravveji commented Jun 7, 2024

In this PR:

Remarks:

  • As of Brotli/1.1.0-GCCcore-13.2.0 (compared to v.1.0.9), the libbrotli*-static.a are no longer built. As a result of that, building Brunsli/0.1-GCCcore-13.2.0 fails. To fix this, a new patch is introduced (Brunsli-0.1-fix-Brotli-static-library-names.patch)
  • This PR introduces a new .eb for libgeotiff, Brunsli, GEOS, GDAL, and so does PR {lib}[GCCcore/13.2.0] Brunsli v0.1, GDAL v3.9.0, libgeotiff v1.7.3, ... #20675. So, some care has to be practiced here to avoid duplication

@moravveji
Copy link
Author

moravveji commented Jun 12, 2024

@Micket : I have just came across your PR #20675 which contains an easyconfig for GDAL/3.9.0-foss-2023b and libgeotiff/1.7.3-GCCcore-13.2.0. I must have searched deeper before placing this PR. How shall we best avoid a possible conflict between the two when merging?

[UPDATE]: I did a diff on our easyconfigs for GDAL and they are just identical, which makes life easier. The same is true for libgeotiff and GEOS.

@moravveji moravveji changed the title WIP: R-bundle-CRAN-2023b and 12 updated dependencies R-bundle-CRAN-2023b and 12 updated dependencies Jun 14, 2024
@verdurin
Copy link
Member

Test report by @verdurin
FAILED
Build succeeded for 40 out of 43 (12 easyconfigs in total)
easybuild-c7.novalocal - Linux CentOS Linux 7.9.2009, x86_64, Intel Xeon Processor (Skylake, IBRS), Python 3.6.8
See https://gist.github.com/verdurin/5e5f552ce32cc2c9aebb700c3da0c0d3 for a full test report.

@boegel boegel added the update label Jun 19, 2024
@boegel boegel added this to the 4.x milestone Jun 19, 2024
@verdurin
Copy link
Member

verdurin commented Jun 24, 2024

@moravveji - thanks for your contribution and welcome to EasyBuild.
Certainly a bold approach to start with this one...

Could you resolve the conflicts, please, preferably by removing them from your PR and specifying them as dependencies instead.

The build failure I saw was a checksum mismatch.

@boegel
Copy link
Member

boegel commented Jun 24, 2024

To be clear: conflicts here are caused by the merge of #20675

@moravveji
Copy link
Author

Thanks @verdurin and @boegel for your comments. I have few questions about some (minor?) issues with this PR:

  • libopus gives me this error (truncated):
    == 2024-06-19 09:07:10,961 filetools.py:837 WARNING IOError occurred while trying to download https://downloads.xiph.org/releases/opus/opus-1.5.2.tar.gz to /eb/maint/sources/l/libopus/opus-1.5.2.tar.gz: <urlopen error [SSL: TLSV1_ALERT_PROTOCOL_VERSION] tlsv1 alert protocol version (_ssl.c:877)>
    == 2024-06-19 09:07:11,430 build_log.py:171 ERROR EasyBuild crashed with an error (at easybuild/base/exceptions.py:126 in __init__): Couldn't find file opus-1.5.2.tar.gz anywhere, and downloading it didn't work either... Paths attempted (in order): /tmp/eb-oc9o47pd/files_pr20756/l/libopus/l/libopus/opus-1.5.2.tar.gz, /tmp/eb-oc9o47pd/files_pr20756/l/libopus/libopus/opus-1.5.2.tar.gz, /tmp/eb-oc9o47pd/files_pr20756/l/libopus/opus-1.5.2.tar.gz, /home/software/src/easybuild/local/l/libopus/opus-1.5.2.tar.gz, /home/software/src/easybuild/local/libopus/opus-1.5.2.tar.gz, /home/software/src/easybuild/local/opus-1.5.2.tar.gz, /eb/maint/local/l/libopus/opus-1.5.2.tar.gz, /eb/maint/local/libopus/opus-1.5.2.tar.gz, /eb/maint/local/opus-1.5.2.tar.gz, /home/software/src/github/easybuild-easyconfigs/easybuild/easyconfigs/l/libopus/opus-1.5.2.tar.gz, /home/software/src/github/easybuild-easyconfigs/easybuild/easyconfigs/libopus/opus-1.5.2.tar.gz, /home/software/src/github/easybuild-easyconfigs/easybuild/easyconfigs/opus-1.5.2.tar.gz, /eb/maint/software/EasyBuild/4.9.2/easybuild/easyconfigs/l/libopus/opus-1.5.2.tar.gz, /eb/maint/software/EasyBuild/4.9.2/easybuild/easyconfigs/libopus/opus-1.5.2.tar.gz, /eb/maint/software/EasyBuild/4.9.2/easybuild/easyconfigs/opus-1.5.2.tar.gz, /home/software/src/github/easybuild-easyconfigs/easybuild/easyconfigs/l/libopus/opus-1.5.2.tar.gz, /home/software/src/github/easybuild-easyconfigs/easybuild/easyconfigs/libopus/opus-1.5.2.tar.gz, /home/software/src/github/easybuild-easyconfigs/easybuild/easyconfigs/opus-1.5.2.tar.gz, /tmp/eb-oc9o47pd/files_pr20756/l/libopus/opus-1.5.2.tar.gz, /tmp/eb-oc9o47pd/files_pr20756/libopus/opus-1.5.2.tar.gz, /tmp/eb-oc9o47pd/files_pr20756/opus-1.5.2.tar.gz, /home/software/src/github/easybuild-easyconfigs/easybuild/easyconfigs/l/libopus/opus-1.5.2.tar.gz, /home/software/src/github/easybuild-easyconfigs/easybuild/easyconfigs/libopus/opus-1.5.2.tar.gz, /home/software/src/github/easybuild-easyconfigs/easybuild/easyconfigs/opus-1.5.2.tar.gz, /eb/maint/software/EasyBuild/4.9.2/easybuild/easyconfigs/l/libopus/opus-1.5.2.tar.gz, /eb/maint/software/EasyBuild/4.9.2/easybuild/easyconfigs/libopus/opus-1.5.2.tar.gz, /eb/maint/software/EasyBuild/4.9.2/easybuild/easyconfigs/opus-1.5.2.tar.gz, /eb/maint/sources/l/libopus/opus-1.5.2.tar.gz, /eb/maint/sources/libopus/opus-1.5.2.tar.gz, /eb/maint/sources/opus-1.5.2.tar.gz, https://downloads.xiph.org/releases/opus/opus-1.5.2.tar.gz, https://sources.easybuild.io/l/libopus/opus-1.5.2.tar.gz  (at easybuild/framework/easyblock.py:975 in obtain_file)
    
    However, the very correct download URL is in the list: https://downloads.xiph.org/releases/opus/opus-1.5.2.tar.gz. So, I am not sure how to fix this
  • The PR {lib}[GCCcore/13.2.0] Brunsli v0.1, GDAL v3.9.0, libgeotiff v1.7.3, ... #20675 was put in before this PR, and it contained GEOS, libgeotiff and GDAL which I have removed from the current PR (commit 9059f2a). However, to build Brunsli against Brotli, a patch is needed, which is included in the current PR, and is missing from the other one. That is why I have not removed Brunsli from here.
    How do you propose to proceed here?
  • For the R-bundle-CRAN itself, I get the following error:
    == 2024-06-19 10:09:58,752 easyblock.py:4285 WARNING build failed (first 300 chars): Checksum verification for extension source signal_1.8-0.tar.gz failed
    
    I know as a fact that the developers/maintainers of the R package signal have changed the source while keeping the version number. As a result, the fingerprint has changed under the hood. Because the same package is used in older versions of R-bundle-CRAN module, on most systems, the older signal-1.8-0.tar.gz source file is found from the cached source paths with the outdated fingerprint. Therefore, the checksum fails here. The easy solution I found was to remove the source .tar.gz file and use eb --force --inject-checksums=sha256 ... to get an updated fingerprint. So, unless the source is manually removed from all test systems, this step is always gonna fail

Thanks in advance

@Flamefire
Copy link
Contributor

  == 2024-06-19 09:07:10,961 filetools.py:837 WARNING IOError occurred while trying to download https://downloads.xiph.org/releases/opus/opus-1.5.2.tar.gz to /eb/maint/sources/l/libopus/opus-1.5.2.tar.gz: <urlopen error [SSL: TLSV1_ALERT_PROTOCOL_VERSION] tlsv1 alert protocol version (_ssl.c:877)>
  However, the very correct download URL is in the list: `https://downloads.xiph.org/releases/opus/opus-1.5.2.tar.gz`. So, I am not sure how to fix this

Looks like an SSL issue. Might be intermediate.

* The PR [{lib}[GCCcore/13.2.0] Brunsli v0.1, GDAL v3.9.0, libgeotiff v1.7.3, ... #20675](https://github.com/easybuilders/easybuild-easyconfigs/pull/20675) was put in before this PR, and it contained GEOS, libgeotiff and GDAL which I have removed from the current PR (commit [9059f2a](https://github.com/easybuilders/easybuild-easyconfigs/commit/9059f2a554da79bd24ec9e84cf3fe829197e68df)). However, to build `Brunsli` against `Brotli`, a patch is needed, which is included in the current PR, and is missing from the other one. That is why I have not removed Brunsli from here.
  How do you propose to proceed here?

The patch is not required as it is handled in the EC you mentioned: https://github.com/easybuilders/easybuild-easyconfigs/pull/20675/files#diff-0ea948e4665fcb18be3b5d531fc2c022b3124c586df87e5cb05eea1c7f69b268R30

However I think the patch is clearer than the sed used there. I suggest to merge current develop into your branch, remove the sed line and instead add your patch (needs to be done during resolving the merge conflict)
This should result in a small change only to that EC.

  ```
  == 2024-06-19 10:09:58,752 easyblock.py:4285 WARNING build failed (first 300 chars): Checksum verification for extension source signal_1.8-0.tar.gz failed
  ```
  I know as a fact that the developers/maintainers of the R package `signal` have changed the source while keeping the version number. As a result, the fingerprint has changed under the hood. Because the same package is used in older versions of `R-bundle-CRAN` module, on most systems, the older `signal-1.8-0.tar.gz` source file is found from the cached source paths with the outdated fingerprint. Therefore, the checksum fails here. The easy solution I found was to remove the source .tar.gz file and use `eb --force --inject-checksums=sha256 ...` to get an updated fingerprint. So, unless the source is manually removed from all test systems, this step is always gonna fail

3 steps:

  • Open an issue with the maintainers of that package telling them not to do that (again)
  • Compare the new and old version of the archive contents
  • If they are the same just add an alternate checksum : 'source': ('checksum1', 'checksum2'),

@Flamefire
Copy link
Contributor

@moravveji I created a PR to resolve the conflicts stopping this from being merged: moravveji#1

Feel free to handle it yourself, just wanted to offer it to get this merged as we like to use it at our site too. So thanks for the effort of updating this monstrous EC

@moravveji
Copy link
Author

@moravveji I created a PR to resolve the conflicts stopping this from being merged: moravveji#1

Feel free to handle it yourself, just wanted to offer it to get this merged as we like to use it at our site too. So thanks for the effort of updating this monstrous EC

Thanks @Flamefire for your new branch. I think I already handled the conflicts in my current branch, by updating the develop branch in my own fork. Yet, I have to remove more EC from this PR with another commit, which I will do once the first round of checks are completed.

For signal-1.8-0.tar.gz I compared (diff) the extracted old and new sources, and the difference between the two is only in the sources/DESCRIPTION file. So, it is pretty safe to have two checksums for a single source filename. If I understood your comment correctly, then the accepted syntax for two checksums (or even more) per one extension source can look like the following, right?

    ('signal', '1.8-0', {
        'checksums': ['0a604949bae91410a150a22cfa02d954f5b83166cc7a73e5409554d00e0417a7', # old source file
                      '23ff303856ff09734e8ef280cec94556a15cdc702727ae5d02f8cfa4d8f473b7'], # new source file
    }),

@Flamefire
Copy link
Contributor

For signal-1.8-0.tar.gz I compared (diff) the extracted old and new sources, and the difference between the two is only in the sources/DESCRIPTION file. So, it is pretty safe to have two checksums for a single source filename. If I understood your comment correctly, then the accepted syntax for two checksums (or even more) per one extension source can look like the following, right?

    ('signal', '1.8-0', {
        'checksums': ['0a604949bae91410a150a22cfa02d954f5b83166cc7a73e5409554d00e0417a7', # old source file
                      '23ff303856ff09734e8ef280cec94556a15cdc702727ae5d02f8cfa4d8f473b7'], # new source file
    }),

Almost: it must be a tuple, not a list, i.e. "()" instead of "[]"

@moravveji
Copy link
Author

This should be easy ;-)
In the EC for Brunsli, I have a long line for the checksum, which leads to the following error:

/home/runner/work/easybuild-easyconfigs/easybuild-easyconfigs/easybuild/easyconfigs/b/Brunsli/Brunsli-0.1-GCCcore-13.2.0.eb:20:10: E127 continuation line over-indented for visual indent

I have tried different ways to get around this (such as using \ for line continuation, or using newline and simply splitting the line into two), but so far, no success. The line looks like this:

checksums = [
    {'v0.1.tar.gz': '62762dc740f9fcc9706449c078f12c2a366416486d2882be50a9f201f99ac0bc'},
    {'Brunsli-0.1-fix-Brotli-static-library-names.patch': \
         '0b921fb4839f0b0223c1bf08499e84afd5d428f448664fd1337173f45a815704'},
]

Can someone plz give an advice?

Co-authored-by: Alexander Grund <[email protected]>
@moravveji
Copy link
Author

moravveji commented Jun 26, 2024

    ('signal', '1.8-0', {
        'checksums': ['0a604949bae91410a150a22cfa02d954f5b83166cc7a73e5409554d00e0417a7', # old source file
                      '23ff303856ff09734e8ef280cec94556a15cdc702727ae5d02f8cfa4d8f473b7'], # new source file
    }),

Almost: it must be a tuple, not a list, i.e. "()" instead of "[]"

Well, seems like EB is clever enough to spot the trick, and refuse to accept it. I get this error now:

AssertionError: False is not true : No checksum issues:
Checksums missing for one or more sources/patches of extension signal in R-bundle-CRAN-2024.06-foss-2023b.eb: found 1 sources + 0 patches vs 2 checksums

Makes me think if it is legitimate to make an exception here, and remove the checksums only for this specific extension (signal), given that the checksums are strongly recommended, but not enforced.

@Flamefire
Copy link
Contributor

Flamefire commented Jun 27, 2024

Makes me think if it is legitimate to make an exception here, and remove the checksums only for this specific extension (signal), given that the checksums are strongly recommended, but not enforced.

The syntax was wrong, my mistake, sorry: moravveji#2

Comment on lines 1598 to 1599
'checksums': ('0a604949bae91410a150a22cfa02d954f5b83166cc7a73e5409554d00e0417a7', # older source
'23ff303856ff09734e8ef280cec94556a15cdc702727ae5d02f8cfa4d8f473b7'), # newer source
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'checksums': ('0a604949bae91410a150a22cfa02d954f5b83166cc7a73e5409554d00e0417a7', # older source
'23ff303856ff09734e8ef280cec94556a15cdc702727ae5d02f8cfa4d8f473b7'), # newer source
'checksums': [
('89cba854167a2b051a58cf3b73ccbf74eeb47c890ac39720611cd41f86b94684',
'0a604949bae91410a150a22cfa02d954f5b83166cc7a73e5409554d00e0417a7'),
],

Copy link
Contributor

Choose a reason for hiding this comment

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

Or this, yes. To me it looks better on 2 lines for the single list element though. I also took the liberty to improve the error message seen here: moravveji#2

Copy link
Author

Choose a reason for hiding this comment

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

I see the many green lights again. So, thanks @Flamefire for the nice fix.
All the necessary adjustments after the recent merges are included here.
So, it this PR ready for testing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Test report coming up. I don't expect any issues. Just takes a while to install 1115 extensions ;-)

Copy link
Author

Choose a reason for hiding this comment

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

I have seen surprises at test step ... so, I stay tuned. On our machines it took roughly 6 hours to build (including dependencies).

@moravveji moravveji changed the title R-bundle-CRAN-2023b and 12 updated dependencies R-bundle-CRAN-2023b, Brunsli-0.1 patch, and easyconfigs.py Jun 28, 2024
@Flamefire
Copy link
Contributor

Test report by @Flamefire
SUCCESS
Build succeeded for 25 out of 25 (2 easyconfigs in total)
n1336 - Linux RHEL 8.7 (Ootpa), x86_64, Intel(R) Xeon(R) Platinum 8470 (icelake), Python 3.8.13
See https://gist.github.com/Flamefire/bed936187da64cede291f782af3e27ee for a full test report.

@Flamefire
Copy link
Contributor

After #20898 was merged this only partially applies now.

  • R-bundle is likely the same as the merged one, but might be wort to compare
  • Brunsli is still using the sed approach instead of the patch
  • The minor fix to the EC tests

Not sure if it is worth following up on this although I would. Will be a very small PR I guess.

@moravveji
Copy link
Author

after the other PR was merged, I got disappointed, and lost interest in this one. So, please feel free to either follow it up or close it. Thanks in advance.

@Flamefire
Copy link
Contributor

I added the remaining changes of yours to #21180 and #21181, so you can close this PR

@moravveji
Copy link
Author

@Flamefire : Thanks for that.

@moravveji moravveji closed this Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants